Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1078483002:
[es6]: Lexical arguments for arrow functions (Closed)
|
Created:
5 years, 8 months ago by arv (Not doing code reviews) Modified:
5 years, 8 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionLexical arguments for arrow functions
Only allocate 'arguments' if the scope is not an arrow scope.
BUG=v8:2700
LOG=N
R=adamk@chromium.org, wingo@igalia.cmo
Committed: https://crrev.com/635b5fea9879de29173376ee12ae12319370123e
Cr-Commit-Position: refs/heads/master@{#27716}
Patch Set 1 #Patch Set 2 : ready #
Total comments: 5
Patch Set 3 : Make arguments() DCHECK instead #
Messages
Total messages: 25 (4 generated)
arv@chromium.org changed reviewers: + adamk@chromium.org, wingo@igalia.com
Adam, Andy: Is this a reasonable approach? I can make this into a real CL if you think this approach is sound.
This looks reasonable to me (though I'm just now booting up on arrow functions).
On 2015/04/08 21:59:00, adamk wrote: > This looks reasonable to me (though I'm just now booting up on arrow functions). Looks reasonable to me too.
ready
This is now ready for realz. PTAL
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { Should there be a test where a parameter name of the arrow function is "arguments"? IIRC that's not illegal in arrow functions
On 2015/04/09 15:13:09, caitp wrote: > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > Should there be a test where a parameter name of the arrow function is > "arguments"? IIRC that's not illegal in arrow functions Our parser does not allow (arguments) => {} maybe that is a bug... I'm looking at the spec...
On 2015/04/09 15:37:40, arv wrote: > On 2015/04/09 15:13:09, caitp wrote: > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): > > > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > > Should there be a test where a parameter name of the arrow function is > > "arguments"? IIRC that's not illegal in arrow functions > > Our parser does not allow > > (arguments) => {} > > maybe that is a bug... I'm looking at the spec... https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi... The arrow parameters are StrictFormalParameters but as far as I can tell the only restriction on StrictFormalParameters is that it does not allow any duplicate bindings.
On 2015/04/09 15:37:40, arv wrote: > On 2015/04/09 15:13:09, caitp wrote: > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): > > > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > > Should there be a test where a parameter name of the arrow function is > > "arguments"? IIRC that's not illegal in arrow functions > > Our parser does not allow > > (arguments) => {} > > maybe that is a bug... I'm looking at the spec... ArrowFormalParameters are StrictFormalParameters (so no duplicates allowed), but they don't imply strict code (so, "eval" and "arguments" are allowed) I think it matters re: lexical `arguments`, because a paramter name arguments should override the lexical arguments. But, I don't think it's a blocker
On 2015/04/09 15:43:44, arv wrote: > On 2015/04/09 15:37:40, arv wrote: > > On 2015/04/09 15:13:09, caitp wrote: > > > > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > > File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): > > > > > > > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > > > Should there be a test where a parameter name of the arrow function is > > > "arguments"? IIRC that's not illegal in arrow functions > > > > Our parser does not allow > > > > (arguments) => {} > > > > maybe that is a bug... I'm looking at the spec... > > https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi... > > The arrow parameters are StrictFormalParameters but as far as I can tell the > only restriction on StrictFormalParameters is that it does not allow any > duplicate bindings. https://code.google.com/p/v8/issues/detail?id=4020
On 2015/04/09 15:44:21, caitp wrote: > I think it matters re: lexical `arguments`, because a paramter name arguments > should override the lexical arguments. But, I don't think it's a blocker Yeah. I don't think the parser bug should block this.
https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h#newcode398 src/scopes.h:398: return is_arrow_scope() ? nullptr : arguments_; Why is this necessary? Reading the code again, I don't see why arguments_ would ever get set on arrow scopes. That is, why is arguments ever getting declared as a local in an arrow scope?
lgtm https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h#newcode398 src/scopes.h:398: return is_arrow_scope() ? nullptr : arguments_; On 2015/04/09 15:56:03, adamk wrote: > Why is this necessary? Reading the code again, I don't see why arguments_ would > ever get set on arrow scopes. That is, why is arguments ever getting declared as > a local in an arrow scope? Sorry, I've been spending too much time in strict code and keep forgetting how other bits of scope resolution work. This is fine.
LGTM. Nice! https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/arrow-functions-lexical-arguments.js (right): https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { On 2015/04/09 15:13:09, caitp wrote: > Should there be a test where a parameter name of the arrow function is > "arguments"? IIRC that's not illegal in arrow functions I think it is illegal because the formals of an arrow function are parsed strictly.
> https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > On 2015/04/09 15:13:09, caitp wrote: > > Should there be a test where a parameter name of the arrow function is > > "arguments"? IIRC that's not illegal in arrow functions > > I think it is illegal because the formals of an arrow function are parsed > strictly. Uf, sorry I missed the conversation outside the comments thing. Nice catch regarding bug 4020; I somehow missed the distinction between StrictFormalParameters and "parameters of a function whose body is strict".
On 2015/04/09 16:22:26, wingo wrote: > > > https://codereview.chromium.org/1078483002/diff/20001/test/mjsunit/harmony/ar... > > test/mjsunit/harmony/arrow-functions-lexical-arguments.js:151: (() => { > > On 2015/04/09 15:13:09, caitp wrote: > > > Should there be a test where a parameter name of the arrow function is > > > "arguments"? IIRC that's not illegal in arrow functions > > > > I think it is illegal because the formals of an arrow function are parsed > > strictly. > > Uf, sorry I missed the conversation outside the comments thing. Nice catch > regarding bug 4020; I somehow missed the distinction between > StrictFormalParameters and "parameters of a function whose body is strict". Same here. This is not the first time I thought StrictFormalParameters is the same as FormalParameters in strict code. https://bugs.ecmascript.org/show_bug.cgi?id=4152
https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1078483002/diff/20001/src/scopes.h#newcode398 src/scopes.h:398: return is_arrow_scope() ? nullptr : arguments_; On 2015/04/09 16:18:04, adamk wrote: > On 2015/04/09 15:56:03, adamk wrote: > > Why is this necessary? Reading the code again, I don't see why arguments_ > would > > ever get set on arrow scopes. That is, why is arguments ever getting declared > as > > a local in an arrow scope? > > Sorry, I've been spending too much time in strict code and keep forgetting how > other bits of scope resolution work. This is fine. Changing this to a DCHECK instead.
Make arguments() DCHECK instead
The CQ bit was checked by arv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1078483002/#ps40001 (title: "Make arguments() DCHECK instead")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078483002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/635b5fea9879de29173376ee12ae12319370123e Cr-Commit-Position: refs/heads/master@{#27716} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
