[webkit-reviews] review granted: [Bug 64466] Refactor JSC to replace JSCell::operator new with static create method : [Attachment 100755] 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 100755: Patch
https://bugs.webkit.org/attachment.cgi?id=100755&action=review

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


This looks good.

There are some calls to new inside WebCore/bridge/qt/qt_runtime.cpp that need
to be changed to use create instead.

review+ but you’ll need to at least fix the Qt build issue.

> Source/JavaScriptCore/jsc.cpp:147
> +protected:
>      GlobalObject(JSGlobalData&, Structure*, const Vector<UString>&
arguments);

I suggest you make these constructors private rather than protected in classes
where there is no derived class. Generally speaking we would like to use
private more often than protected.

> Source/JavaScriptCore/jsc.cpp:152
> +	   return new (allocateCell<GlobalObject>(&(globalData.heap)))
GlobalObject(globalData, structure, arguments);

This has one extra set of parentheses, around globalData.heap, and I think it
would read better without them. There are similar extra parentheses in other
places in the patch.

> Source/JavaScriptCore/API/JSCallbackObject.h:123
> +    // pseudo-placement version of operator new
> +    void* operator new(size_t, void* ptr) { return ptr; }

Why is this needed? What about the real placement new? Why can’t we use that?

This comment is a “what” comment, and instead we want our comments to be “why”
comments. Also we normally want our comments to be sentence style with a first
capital letter and a period.

> Source/JavaScriptCore/runtime/JSCell.h:379
> +    template <typename T> void* allocateCell(Heap* heap)

I think we should use a reference to a heap here instead of a pointer to a
heap.