Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(60)

Issue 1508933004: [es6] support AssignmentPattern as LHS in for-in/of loops (Closed)

Created:
5 years ago by caitp (gmail)
Modified:
5 years ago
Reviewers:
adamk, rossberg
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] support AssignmentPattern as LHS in for-in/of loops BUG=v8:811, v8:4599 LOG=N R=adamk@chromium.org, rossberg@chromium.org Committed: https://crrev.com/e47bdb775564b2cd8365047425898ab4274190a6 Cr-Commit-Position: refs/heads/master@{#32773}

Patch Set 1 #

Patch Set 2 : Make assign_each() part of body in ForInStatement #

Patch Set 3 : Don't reuse VariableProxy ast node #

Total comments: 8

Patch Set 4 : Fixups #

Patch Set 5 : Further fixups #

Total comments: 5

Patch Set 6 : Add cctests to ensure unity between parsers #

Total comments: 1

Patch Set 7 : slightly modify AssignFeedbackVectorSlots + add TODO #

Patch Set 8 : Fix valgrind issue #

Patch Set 9 : Fix test failures #

Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -37 lines) Patch
M src/ast/ast.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M src/parsing/parser.h View 1 2 3 4 2 chunks +8 lines, -6 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 9 chunks +44 lines, -16 lines 0 comments Download
M src/parsing/pattern-rewriter.cc View 1 2 3 4 2 chunks +15 lines, -3 lines 0 comments Download
M src/parsing/preparser.cc View 1 chunk +13 lines, -4 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 4 chunks +24 lines, -8 lines 0 comments Download
M test/mjsunit/harmony/destructuring-assignment.js View 1 chunk +52 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (17 generated)
caitp (gmail)
quick hack to add this. The bit for ForInStatement is probably the most iffy, and ...
5 years ago (2015-12-08 04:24:22 UTC) #1
adamk
The need for special handling in code generation is a red flag to me. I ...
5 years ago (2015-12-10 00:53:25 UTC) #2
caitp (gmail)
On 2015/12/10 00:53:25, adamk wrote: > The need for special handling in code generation is ...
5 years ago (2015-12-10 09:38:22 UTC) #3
caitp (gmail)
On 2015/12/10 09:38:22, caitp wrote: > On 2015/12/10 00:53:25, adamk wrote: > > The need ...
5 years ago (2015-12-10 10:12:31 UTC) #4
adamk
Looking good. Seems like there's some opportunity for cleaning up the PatternRewriter API. https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc File ...
5 years ago (2015-12-10 17:28:03 UTC) #12
caitp (gmail)
let me know how that looks --- I'm not 100% it's great that the two ...
5 years ago (2015-12-10 18:19:36 UTC) #13
adamk
lgtm % test-parsing tests https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1508933004/diff/140001/src/parsing/parser.cc#newcode3402 src/parsing/parser.cc:3402: scope_->NewTemporary(ast_value_factory()->dot_result_string()); On 2015/12/10 18:19:36, caitp ...
5 years ago (2015-12-10 19:23:34 UTC) #14
adamk
https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140 src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) { Do you still need this check?
5 years ago (2015-12-10 20:08:15 UTC) #15
caitp (gmail)
https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140 src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) { On 2015/12/10 20:08:15, adamk wrote: > ...
5 years ago (2015-12-10 20:32:25 UTC) #16
adamk
https://codereview.chromium.org/1508933004/diff/200001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1508933004/diff/200001/src/parsing/parser.cc#newcode3410 src/parsing/parser.cc:3410: each = factory()->NewVariableProxy(temp); It looks like each is always ...
5 years ago (2015-12-10 22:44:17 UTC) #17
adamk
lgtm with a TODO https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140 src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) { On 2015/12/10 ...
5 years ago (2015-12-10 23:03:31 UTC) #18
caitp (gmail)
https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/1508933004/diff/180001/src/ast/ast.cc#newcode140 src/ast/ast.cc:140: if (each()->IsValidReferenceExpression()) { On 2015/12/10 23:03:31, adamk wrote: > ...
5 years ago (2015-12-10 23:18:18 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508933004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508933004/220001
5 years ago (2015-12-10 23:18:40 UTC) #22
caitp (gmail)
On 2015/12/10 23:18:40, commit-bot: I haz the power wrote: > CQ is trying da patch. ...
5 years ago (2015-12-11 00:07:04 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508933004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508933004/260001
5 years ago (2015-12-11 00:07:22 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/13035)
5 years ago (2015-12-11 00:20:34 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508933004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508933004/280001
5 years ago (2015-12-11 00:29:12 UTC) #32
commit-bot: I haz the power
Committed patchset #9 (id:280001)
5 years ago (2015-12-11 01:06:39 UTC) #34
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/e47bdb775564b2cd8365047425898ab4274190a6 Cr-Commit-Position: refs/heads/master@{#32773}
5 years ago (2015-12-11 01:06:54 UTC) #36
adamk
5 years ago (2015-12-11 01:59:12 UTC) #37
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:280001) has been created in
https://codereview.chromium.org/1511773009/ by adamk@chromium.org.

The reason for reverting is: Hits unreachable code (found by fuzzer). Example
crasher:

"for(();;);".

Powered by Google App Engine
This is Rietveld 408576698