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

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


> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1738
> +    JSActivation* activation =
JSActivation::create(&(callFrame->globalData()), callFrame,
static_cast<FunctionExecutable*>(ownerExecutable()));

This still has extra parentheses.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5070
> +	   JSValue arguments =
JSValue(Arguments::create(&(callFrame->globalData()), functionCallFrame));

This still has extra parentheses.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5076
> +    Arguments* arguments =
Arguments::create(&(functionCallFrame->globalData()), functionCallFrame);

This still has extra parentheses.

> Source/JavaScriptCore/jit/JITStubs.cpp:2065
> +    JSActivation* activation =
JSActivation::create(&(stackFrame.callFrame->globalData()),
stackFrame.callFrame,
static_cast<FunctionExecutable*>(stackFrame.callFrame->codeBlock()->ownerExecut
able()));

This still has extra parentheses.

> Source/JavaScriptCore/runtime/Arguments.h:72
> +	   static Arguments* create(JSGlobalData* globalData, CallFrame*
callFrame, NoParametersType noParams)
> +	   {
> +	       return new (allocateCell<Arguments>(globalData->heap))
Arguments(callFrame, noParams);
> +	   }

For "new" we need to use an extra argument because it’s a way to make a named
constructor.

But for a create function, we can actually name the create function in a way
that makes clear what these two different types of creation are and thus we
don’t need to use the “extra argument” technique.

> Source/JavaScriptCore/runtime/JSActivation.cpp:223
> +    JSValue arguments =
JSValue(Arguments::create(&(callFrame->globalData()), callFrame));

This still has extra parentheses.

> Source/JavaScriptCore/runtime/JSString.h:351
> +	   static JSString* create(JSGlobalData* globalData, const UString&
value, HasOtherOwnerType hasOtherOwnerType)
> +	   {
> +	       return new (allocateCell<JSString>(globalData->heap))
JSString(globalData, value, hasOtherOwnerType);
> +	   }
> +	   static JSString* create(JSGlobalData* globalData,
PassRefPtr<StringImpl> value, HasOtherOwnerType hasOtherOwnerType)
> +	   {
> +	       return new (allocateCell<JSString>(globalData->heap))
JSString(globalData, value, hasOtherOwnerType);
> +	   }

Same comment here as above. In a create function we can use a sensible function
name rather than an extra parameter as the way to implement alternate
constructors.

This lets us limit the awkward “extra parameter” technique to local use inside
a class, and the type used for that technique can even be private to the class.


> Source/JavaScriptCore/runtime/NumberObject.h:30
>	   explicit NumberObject(JSGlobalData&, Structure*);

It makes no sense to have “explicit” on a constructor with multiple arguments,
so we really ought to remove this.