RESOLVED FIXED 128031
Simplified name scope creation for function expressions
https://bugs.webkit.org/show_bug.cgi?id=128031
Summary Simplified name scope creation for function expressions
Geoffrey Garen
Reported 2014-01-31 17:12:56 PST
Simplified name scope creation for function expressions
Attachments
Patch (34.09 KB, patch)
2014-01-31 17:29 PST, Geoffrey Garen
mark.lam: review+
Geoffrey Garen
Comment 1 2014-01-31 17:29:40 PST
WebKit Commit Bot
Comment 2 2014-01-31 17:32:21 PST
Attachment 222866 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSNameScope.h:77: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Lam
Comment 3 2014-01-31 19:10:30 PST
Comment on attachment 222866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222866&action=review Other than the part in isNumericCompareFunction() which I'm unsure about, everything else looks good to me. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:86 > + JSScope* scope = callData.js.scope; > > - JSObject* error = executable->prepareForExecution(exec, callData.js.scope, CodeForCall); > + JSObject* error = executable->prepareForExecution(exec, jsCast<JSFunction*>(function), &scope, CodeForCall); Is there a reason you didn't want prepareForExecution() to update the scope in callData.js.scope? I'm not sure which (to update or not to update) is the right thing to do yet, but thought I'd bring this to your attention and ask in case this was not intentional.
Geoffrey Garen
Comment 4 2014-01-31 19:41:16 PST
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:86 > > + JSScope* scope = callData.js.scope; > > > > - JSObject* error = executable->prepareForExecution(exec, callData.js.scope, CodeForCall); > > + JSObject* error = executable->prepareForExecution(exec, jsCast<JSFunction*>(function), &scope, CodeForCall); > > Is there a reason you didn't want prepareForExecution() to update the scope in callData.js.scope? I'm not sure which (to update or not to update) is the right thing to do yet, but thought I'd bring this to your attention and ask in case this was not intentional. callData.js.scope is const, so we can't update it. It's const so a class can return the same piece of data each time, if it likes. In this case, we don't need to update our scope because we don't use it. We're only compiling in order to analyze the code -- we're not running it.
Mark Lam
Comment 5 2014-01-31 22:06:08 PST
(In reply to comment #4) > callData.js.scope is const, so we can't update it. It's const so a class can return the same piece of data each time, if it likes. I should have noticed the const. > In this case, we don't need to update our scope because we don't use it. We're only compiling in order to analyze the code -- we're not running it. Sounds good. r=me.
Geoffrey Garen
Comment 6 2014-02-03 12:34:47 PST
Note You need to log in before you can comment on or make changes to this bug.