[webkit-reviews] review denied: [Bug 20422] Patch to allow custom memory allocation control : [Attachment 23971] proposed patch for JavaScripCore

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Zoltan Horvath
<Horvath.Zoltan.6 at stud.u-szeged.hu>'s request for review:
Bug 20422: Patch to allow custom memory allocation control
https://bugs.webkit.org/show_bug.cgi?id=20422

Attachment 23971: proposed patch for JavaScripCore
https://bugs.webkit.org/attachment.cgi?id=23971&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
(In reply to comment #16)
>  - There were several non-public inheritances therefore we changed their
> visibility to public (the main problem is that classes non-publicly inherit
> from Noncopyable)

Why did you change the visibility to public? That sounds wrong. Using private
inheritance is a valid technique and you shouldn't just change the inheritance.


>  - Constructor was added to several structs and they are called when the
> structs are instantiated

Why?

>  - Code-generator scripts were changed to use the constructors of the structs

> when instantiating them in static arrays 
>     for example:
>      -const ClassInfo InternalFunction::info = { "Function", 0, 0, 0 };
>      +const ClassInfo InternalFunction::info("Function", 0, 0, 0);

This change is unacceptable.

On many platforms, including Mac OS X, it will cause the code of the
JavaScriptCore framework to be paged in during load time so the constructors
can be run. Struct initialization doesn't require code execution and is handled
by the loader, but constructor execution does. This has a dramatic effect on
the load time performance of an application that links with the library before
it uses the library.

It seems that there are multiple unrelated changes here. I don't see how a
change to ClassInfo has anything to do with the New.h header.