Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1274193004:
Let eval scope (Closed)
|
Created:
5 years, 4 months ago by Dan Ehrenberg Modified:
5 years, 4 months ago Reviewers:
adamk CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse a new lexical context for sloppy-mode eval
In ES6, direct eval() in sloppy mode uses the enclosing function-level
("var") scope for var-style bindings and a new lexical scope for lexical
bindings like let and class. This patch implements that feature by making
lexical bindings that are directly within an EVAL_SCOPE be on the local
scope rather than the enclosing one.
BUG=v8:4288
LOG=Y
R=adamk
Committed: https://crrev.com/d03191beb1888b272e51912ebc45c4e8794f2060
Cr-Commit-Position: refs/heads/master@{#30120}
Patch Set 1 #Patch Set 2 : Fix webkit test similar to in trunk #
Total comments: 1
Patch Set 3 : Fix another test #
Total comments: 7
Patch Set 4 : Changes for code review #
Messages
Total messages: 35 (17 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4801)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/8436)
adamk@chromium.org changed reviewers: + adamk@chromium.org
https://codereview.chromium.org/1274193004/diff/20001/test/webkit/class-synta... File test/webkit/class-syntax-name-expected.txt (right): https://codereview.chromium.org/1274193004/diff/20001/test/webkit/class-synta... test/webkit/class-syntax-name-expected.txt:111: FAIL var result = A; class A {}; result should throw an exception. Was undefined. This is the line that's differing, it now passes.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2016 src/parser.cc:2016: // Similarly, strict mode eval scope does not leak variable declarations to This comment's getting pretty outdated, but if you want to leave it here this sentence should also mention lexical variables in sloppy mode. https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2019 src/parser.cc:2019: declaration_scope->is_strict_eval_scope() || I think it would be clearer to get rid of this line (and the is_strict_eval_scope method, which is only used here)... https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2023 src/parser.cc:2023: (declaration_scope->is_eval_scope() && IsLexicalVariableMode(mode))) { ...and replace this condition with: (declaration_scope->is_eval_scope() && (is_strict(language_mode()) || IsLexicalVariableMode(mode))) That makes it clear when eval is handled here. https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2104 src/parser.cc:2104: !IsLexicalVariableMode(mode)) { It's weird that this isn't part of an else/if clause of the above if statement, but I suppose it's not any weirder now than it was before.
The CQ bit was checked by littledan@chromium.org
https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2019 src/parser.cc:2019: declaration_scope->is_strict_eval_scope() || On 2015/08/11 at 01:17:43, adamk wrote: > I think it would be clearer to get rid of this line (and the is_strict_eval_scope method, which is only used here)... Done https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2023 src/parser.cc:2023: (declaration_scope->is_eval_scope() && IsLexicalVariableMode(mode))) { On 2015/08/11 at 01:17:43, adamk wrote: > ...and replace this condition with: > > (declaration_scope->is_eval_scope() && > (is_strict(language_mode()) || IsLexicalVariableMode(mode))) > > That makes it clear when eval is handled here. Done https://codereview.chromium.org/1274193004/diff/40001/src/parser.cc#newcode2104 src/parser.cc:2104: !IsLexicalVariableMode(mode)) { On 2015/08/11 at 01:17:43, adamk wrote: > It's weird that this isn't part of an else/if clause of the above if statement, but I suppose it's not any weirder now than it was before. Good point; moved it to an else clause above. The legacy const code still looks like it could be cleaned up, but I'll leave that for another day (I'm not sure how that's supposed to interact with redeclaration, or why it's restricted to script scope).
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/60001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274193004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274193004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d03191beb1888b272e51912ebc45c4e8794f2060 Cr-Commit-Position: refs/heads/master@{#30120} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
