Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1747853002:
Fix issue with re-scoping in do expressions (Closed)
DescriptionFix issue with re-scoping in do expressions
R=rossberg@chromium.org
BUG=v8:4783
LOG=N
Committed: https://crrev.com/de817ef9c7b37e6aa01948c3063723e61af0e756
Cr-Commit-Position: refs/heads/master@{#34382}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (8 generated)
lgtm
Description was changed from ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 ========== to ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 ==========
nikolaos@chromium.org changed reviewers: + adamk@chromium.org
Adam, would you like to take a quick look at this? There is a related discussion at: https://bugs.chromium.org/p/v8/issues/detail?id=4783
lgtm for now, agreed that this is sufficient for the case at hand https://codereview.chromium.org/1747853002/diff/1/src/parsing/parameter-initi... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/1747853002/diff/1/src/parsing/parameter-initi... src/parsing/parameter-initializer-rewriter.cc:65: if (var->mode() != TEMPORARY) return; I agree that this seems fine for the ".catch" variable for now (and would also be fine for lexical scopes). I still think there's more work to do for properly handling var-declarations in do-expressions, but I don't think that would end up in VisitVariableProxy (they're never eagerly-resolved anyway).
The CQ bit was checked by nikolaos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747853002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1747853002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11680)
Description was changed from ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 ========== to ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 LOG=N ==========
The CQ bit was checked by nikolaos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747853002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1747853002/1
Message was sent while issue was closed.
Description was changed from ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 LOG=N ========== to ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 LOG=N ========== to ========== Fix issue with re-scoping in do expressions R=rossberg@chromium.org BUG=v8:4783 LOG=N Committed: https://crrev.com/de817ef9c7b37e6aa01948c3063723e61af0e756 Cr-Commit-Position: refs/heads/master@{#34382} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/de817ef9c7b37e6aa01948c3063723e61af0e756 Cr-Commit-Position: refs/heads/master@{#34382}
Message was sent while issue was closed.
https://codereview.chromium.org/1747853002/diff/1/src/parsing/parameter-initi... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/1747853002/diff/1/src/parsing/parameter-initi... src/parsing/parameter-initializer-rewriter.cc:65: if (var->mode() != TEMPORARY) return; On 2016/02/29 19:20:41, adamk wrote: > I agree that this seems fine for the ".catch" variable for now (and would also > be fine for lexical scopes). I still think there's more work to do for properly > handling var-declarations in do-expressions, but I don't think that would end up > in VisitVariableProxy (they're never eagerly-resolved anyway). Agreed. Var decls are the only bit missing towards full support for do-expressions at this point, AFAICT. If I find some time, I may look into that. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
