Bug 67174 – Unzip initialization lists and constructors in JSCell hierarchy (4/7)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

This is the fourth level of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.

[Mark Hahnenberg]

Created attachment 105687 [details]
Patch

[Mark Hahnenberg]

Created attachment 105705 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 105705 [details]
Patch

Attachment 105705 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9572543

[Mark Hahnenberg]

Created attachment 105803 [details]
Patch

[Darin Adler]

Comment on attachment 105803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105803&action=review

> Source/JavaScriptCore/runtime/JSArray.h:77
> +        static JSArray* create(JSGlobalData&, Structure*, unsigned initialLength, ArrayCreationMode createMode);

The argument name here, createMode, is not helpful and should be omitted.

> Source/JavaScriptCore/runtime/JSFunction.h:55
> +        static JSFunction* create(ExecState*, JSGlobalObject*, Structure*, int length, const Identifier& name, NativeFunction);

Is making these functions non-inline a good choice speed-wise?

> Source/JavaScriptCore/runtime/JSStaticScopeObject.h:53
> +        void finishCreation(ExecState* exec, const Identifier& ident, JSValue value, unsigned attributes)

Normally we try not to abbreviate. So the name here would be identifier rather than ident.

[Mark Hahnenberg]

Created attachment 105874 [details]
Patch

[Mark Hahnenberg]

Created attachment 105882 [details]
Patch

[Darin Adler]

Comment on attachment 105882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105882&action=review

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:82
> +void JSCallbackObject<Parent>::finishCreation(JSGlobalData& globalData)
> +{
> +    UNUSED_PARAM(globalData);

Why not just leave the parameter name out instead? That’s the best way to mark an argument as unused in C++ functions.

> Source/JavaScriptCore/runtime/DateInstance.cpp:46
> +    finishCreation(exec->globalData(), jsNumber(timeClip(time)));

I would think we’d want the jsNumber function call inside the finishCreation function.

[Mark Hahnenberg]

Created attachment 105975 [details]
Fixing review issues

[Mark Hahnenberg]

Created attachment 105990 [details]
Resetting test bindings results

[Mark Hahnenberg]

Created attachment 106010 [details]
Random improvement

[Oliver Hunt]

Comment on attachment 106010 [details]
Random improvement

View in context: https://bugs.webkit.org/attachment.cgi?id=106010&action=review

Hmmm, there are no changelogs :-(

> Source/JavaScriptCore/runtime/InternalFunction.cpp:47
> +    finishCreation(*globalData, globalObject, name);

Is it intentional the InternalFunction retains the finishCreation in its constructor?

[Mark Hahnenberg]

> Hmmm, there are no changelogs :-(

Hmm dunno what happened to them…they were in earlier versions of the patch lol.

> Is it intentional the InternalFunction retains the finishCreation in its constructor?

Yep.  It had its own isolated version of constructorBody() before, and now it's being changed to finishCreation().  InternalFunction is on the 4th level of the hierarchy below JSCell.  If you look at its immediate superclass, JSObjectWithGlobalObject, its finishCreation() was removed from the constructor, meaning that it needs to be pushed down into the constructor body of InternalFunction.

[Mark Hahnenberg]

Created attachment 106051 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106051 [details]
Patch

Clearing flags on attachment: 106051

Committed r94364: <http://trac.webkit.org/changeset/94364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.