Summary: | Unzip initialization lists and constructors in JSCell hierarchy (4/7) | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ggaren, gustavo, oliver, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 67064 | ||||||||||||||||||||||
Bug Blocks: | 66567, 67420 | ||||||||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2011-08-29 18:36:58 PDT
Created attachment 105687 [details]
Patch
Created attachment 105705 [details]
Patch
Comment on attachment 105705 [details] Patch Attachment 105705 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9572543 Created attachment 105803 [details]
Patch
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. Created attachment 105874 [details]
Patch
Created attachment 105882 [details]
Patch
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. Created attachment 105975 [details]
Fixing review issues
Created attachment 105990 [details]
Resetting test bindings results
Created attachment 106010 [details]
Random improvement
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? > 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. Created attachment 106051 [details]
Patch
Comment on attachment 106051 [details] Patch Clearing flags on attachment: 106051 Committed r94364: <http://trac.webkit.org/changeset/94364> All reviewed patches have been landed. Closing bug. |