Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083083007:
Fix bug with re-scoping arrow function parameter initializers (Closed)
DescriptionFix bug with re-scoping arrow function parameter initializers
When re-scoping arrow function parameter initializers, temporaries
should be moved from the closure of the old scope to the closure of
the new scope, if necessary.
R=adamk@chromium.org, rossberg@chromium.org
BUG=chromium:622663
LOG=N
Committed: https://crrev.com/61c137c8116a800d179ada40570d62a0b70f1d2d
Cr-Commit-Position: refs/heads/master@{#37335}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix test according to reviews #
Total comments: 2
Patch Set 3 : Optimization for closure scopes #
Messages
Total messages: 32 (12 generated)
The CQ bit was checked by nikolaos@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/2083083007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dan (cc:), you may want to take a look at this. https://codereview.chromium.org/2083083007/diff/1/src/parsing/parameter-initi... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/2083083007/diff/1/src/parsing/parameter-initi... src/parsing/parameter-initializer-rewriter.cc:97: if (var->scope() == new_scope_->ClosureScope()) return; The condition of this "if" statement is identical to the previous one, given the DCHECK_EQ above. I was confused by Dan's original comment and changed it in the way that I understand it. Even if the "if" statement in line 97 is completely removed, everything works alright now. However, I think it's better to leave the "if" statement there as an optimization --- no need to remove a temporary and then put it back in the same scope.
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/2083083007/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-622663.js (right): https://codereview.chromium.org/2083083007/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-622663.js:5: +// Flags: --ignition --no-lazy Maybe don't set the --ignition flag - we have a variant which runs all tests with the --ignition flag, so there will be better test coverage without explicitly adding this flag (will test both FCG and Ignition)
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/2083083007/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-622663.js (right): https://codereview.chromium.org/2083083007/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-622663.js:5: +// Flags: --ignition --no-lazy On 2016/06/24 14:34:00, rmcilroy wrote: > Maybe don't set the --ignition flag - we have a variant which runs all tests > with the --ignition flag, so there will be better test coverage without > explicitly adding this flag (will test both FCG and Ignition) Acknowledged.
littledan@chromium.org changed reviewers: + littledan@chromium.org
lgtm This patch seems very reasonable. I especially like all the DCHECKs you added. https://codereview.chromium.org/2083083007/diff/1/src/parsing/parameter-initi... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/2083083007/diff/1/src/parsing/parameter-initi... src/parsing/parameter-initializer-rewriter.cc:97: if (var->scope() == new_scope_->ClosureScope()) return; On 2016/06/24 at 13:39:51, nickie wrote: > The condition of this "if" statement is identical to the previous one, given the DCHECK_EQ above. I was confused by Dan's original comment and changed it in the way that I understand it. > > Even if the "if" statement in line 97 is completely removed, everything works alright now. However, I think it's better to leave the "if" statement there as an optimization --- no need to remove a temporary and then put it back in the same scope. Yeah, this seems like a valid change. I was writing the code imagining that we might eventually extend the scope rewriter to work between arbitrary block scopes, but there are other changes that we would need to this method (and probably others) to enable that.
The CQ bit was checked by nikolaos@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.
Looks good, but one further optimization suggestion https://codereview.chromium.org/2083083007/diff/20001/src/parsing/parameter-i... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/2083083007/diff/20001/src/parsing/parameter-i... src/parsing/parameter-initializer-rewriter.cc:98: int index = old_scope_->ClosureScope()->RemoveTemporary(var); What if the rewriter computed the ClosureScopes of both the old and new scopes during construction? It's a scope chain walk every time otherwise...
https://codereview.chromium.org/2083083007/diff/20001/src/parsing/parameter-i... File src/parsing/parameter-initializer-rewriter.cc (right): https://codereview.chromium.org/2083083007/diff/20001/src/parsing/parameter-i... src/parsing/parameter-initializer-rewriter.cc:98: int index = old_scope_->ClosureScope()->RemoveTemporary(var); On 2016/06/27 18:49:31, adamk wrote: > What if the rewriter computed the ClosureScopes of both the old and new scopes > during construction? It's a scope chain walk every time otherwise... Done.
The CQ bit was checked by nikolaos@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.
Nicki, can we land this before the m53 branch please? This affects Ignition which will ship on certain device configurations in m53.
On 2016/06/28 14:25:46, rmcilroy wrote: > Nicki, can we land this before the m53 branch please? This affects Ignition > which will ship on certain device configurations in m53. Sorry, /s/Nicki/Nickie/
The CQ bit was checked by adamk@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2083083007/#ps40001 (title: "Optimization for closure scopes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug with re-scoping arrow function parameter initializers When re-scoping arrow function parameter initializers, temporaries should be moved from the closure of the old scope to the closure of the new scope, if necessary. R=adamk@chromium.org, rossberg@chromium.org BUG=chromium:622663 LOG=N ========== to ========== Fix bug with re-scoping arrow function parameter initializers When re-scoping arrow function parameter initializers, temporaries should be moved from the closure of the old scope to the closure of the new scope, if necessary. R=adamk@chromium.org, rossberg@chromium.org BUG=chromium:622663 LOG=N Committed: https://crrev.com/61c137c8116a800d179ada40570d62a0b70f1d2d Cr-Commit-Position: refs/heads/master@{#37335} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/61c137c8116a800d179ada40570d62a0b70f1d2d Cr-Commit-Position: refs/heads/master@{#37335} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
