Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634123002:
[parser] Pessimistically assume top-level variables will be assigned. (Closed)
Description[parser] Pessimistically assume top-level variables will be assigned.
We have to pessimistically assume that top-level variables will be assigned.
This is because there may be lazily parsed top-level functions, which, for
efficiency, we preparse without variable tracking.
R=adamk@chromium.org, marja@chromium.org
CC=jarin@chromium.org
BUG=v8:5636
Review-Url: https://codereview.chromium.org/2634123002
Cr-Commit-Position: refs/heads/master@{#42398}
Committed: https://chromium.googlesource.com/v8/v8/+/248d1b3de9872c21df3a92dbaff5c81e2026e0dc
Patch Set 1 #
Total comments: 1
Patch Set 2 : Feedback. #
Total comments: 1
Messages
Total messages: 19 (9 generated)
Description was changed from ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 ========== to ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. Note: I'm not (yet) changing anything for "var" declared variables. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 ==========
Description was changed from ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. Note: I'm not (yet) changing anything for "var" declared variables. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 ========== to ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 ==========
lgtm https://codereview.chromium.org/2634123002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2634123002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:3515: "let foo; function bar() {foo = 42}; ext(bar); ext(foo)", Could you add some destructuring test cases which end up declaring foo? Like let [a, foo] = [0, 0] and so on.
Ahm, I also meant to ask why you ended up putting the maybe_assigned setting to that exact place... is it because that's the logical place to hook into destructuring declarations (or whatever they are called)?
On 2017/01/16 15:28:07, marja wrote: > lgtm > > https://codereview.chromium.org/2634123002/diff/1/test/cctest/test-parsing.cc > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/2634123002/diff/1/test/cctest/test-parsing.cc... > test/cctest/test-parsing.cc:3515: "let foo; function bar() {foo = 42}; ext(bar); > ext(foo)", > Could you add some destructuring test cases which end up declaring foo? > > Like let [a, foo] = [0, 0] and so on. Done.
still lgtm
On 2017/01/16 15:30:03, marja wrote: > Ahm, I also meant to ask why you ended up putting the maybe_assigned setting to > that exact place... is it because that's the logical place to hook into > destructuring declarations (or whatever they are called)? Yes, this seems to be the canonical place since it handles basically all variable declarations.
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 neis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484649493337780,
"parent_rev": "68c994795ec8c449c2dc0991eba815ae7418af06", "commit_rev":
"248d1b3de9872c21df3a92dbaff5c81e2026e0dc"}
Message was sent while issue was closed.
Description was changed from ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 ========== to ========== [parser] Pessimistically assume top-level variables will be assigned. We have to pessimistically assume that top-level variables will be assigned. This is because there may be lazily parsed top-level functions, which, for efficiency, we preparse without variable tracking. R=adamk@chromium.org, marja@chromium.org CC=jarin@chromium.org BUG=v8:5636 Review-Url: https://codereview.chromium.org/2634123002 Cr-Commit-Position: refs/heads/master@{#42398} Committed: https://chromium.googlesource.com/v8/v8/+/248d1b3de9872c21df3a92dbaff5c81e202... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/248d1b3de9872c21df3a92dbaff5c81e202...
Message was sent while issue was closed.
https://codereview.chromium.org/2634123002/diff/20001/src/parsing/pattern-rew... File src/parsing/pattern-rewriter.cc (right): https://codereview.chromium.org/2634123002/diff/20001/src/parsing/pattern-rew... src/parsing/pattern-rewriter.cc:230: // assigned. This is because there may be lazily parsed top-level As discussed offline, this comment is slightly misleading: top-level variables need to be marked as assigned because a script (or indirect eval) could always modify them (unless they're CONST, naturally).
Message was sent while issue was closed.
On 2017/01/17 18:09:04, adamk wrote: > https://codereview.chromium.org/2634123002/diff/20001/src/parsing/pattern-rew... > File src/parsing/pattern-rewriter.cc (right): > > https://codereview.chromium.org/2634123002/diff/20001/src/parsing/pattern-rew... > src/parsing/pattern-rewriter.cc:230: // assigned. This is because there may be > lazily parsed top-level > As discussed offline, this comment is slightly misleading: top-level variables > need to be marked as assigned because a script (or indirect eval) could always > modify them (unless they're CONST, naturally). or unless they are top-level inside a module. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
