[webkit-reviews] review granted: [Bug 64466] Refactor JSC to replace JSCell::operator new with static create method : [Attachment 100909] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 64466: Refactor JSC to replace JSCell::operator new with static create
method
https://bugs.webkit.org/show_bug.cgi?id=64466

Attachment 100909: Patch
https://bugs.webkit.org/attachment.cgi?id=100909&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100909&action=review


Sorry to drive you crazy with new comments each time. I noticed some other
things I missed last time.

> Source/JavaScriptCore/jsc.cpp:146
> -public:
>      GlobalObject(JSGlobalData&, Structure*, const Vector<UString>&
arguments);

We are normally explicit about private even at the top of a class.

> Source/JavaScriptCore/API/JSCallbackObject.h:123
> +    // This version of operator new is necessary because we can't tell
statically 
> +    // that Base will inherit the correct placement version of operator new
from JSCell.

The wording here is confusing. I think what you mean to say is that we would
like to use the placement version of operator new in JSCell, but can’t because
Base is a template argument.

> Source/JavaScriptCore/API/JSContextRef.cpp:100
> +    JSGlobalObject* globalObject =
JSCallbackObject<JSGlobalObject>::create(*globalData.get(), globalObjectClass,
JSCallbackObject<JSGlobalObject>::createStructure(*globalData, jsNull()));

I am surprised that this .get() is needed. Are you sure you needed to add it?

> Source/JavaScriptCore/runtime/ArrayConstructor.h:31
> -    public:
>	   ArrayConstructor(ExecState*, JSGlobalObject*, Structure*,
ArrayPrototype*);

Same issue here, where in this project we would normally make private explicit
rather than depending on the "first things in a class are private" feature.

> Source/JavaScriptCore/runtime/ArrayPrototype.h:-30
> -    public:

Here too.

> Source/JavaScriptCore/runtime/BooleanConstructor.h:-31
> -    public:

Again. I won’t call out the other cases.

> Source/JavaScriptCore/runtime/RegExp.h:82
> +	   // Normally this method would be called create, but we have to avoid
a potential conflict with the 
> +	   // normal public create method that handles caching as well.
> +	   static RegExp* allocateAndInit(JSGlobalData&, const UString&,
RegExpFlags);

Given that description I would name it createWithoutCaching or something like
that.