Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(50)

Issue 1309813007: [es6] implement destructuring assignment (Closed)

Created:
3 years, 8 months ago by caitp (gmail)
Modified:
3 years, 4 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] implement destructuring assignment Attempt #<really big number> Parses, and lazily rewrites Destructuring Assignment expressions. The rewriting strategy involves inserting a placeholder RewritableAssignmentExpression into the AST, whose content expression can be completely rewritten at a later time. Lazy rewriting ensures that errors do not occur due to eagerly rewriting nodes which form part of a binding pattern, thus breaking the meaning of the pattern --- or by eagerly rewriting ambiguous constructs that are not immediately known BUG=v8:811 LOG=Y R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org Committed: https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95 Cr-Commit-Position: refs/heads/master@{#32623}

Patch Set 1 : An implementation #

Total comments: 11

Patch Set 2 : Rebase #

Patch Set 3 : Refactor impl / remove new AST node / more test coverage #

Patch Set 4 : Cache te right scope in DeclareAndInitializeVariables() #

Total comments: 26

Patch Set 5 : Fix lazy compilation + add a bunch more behavioural tests #

Patch Set 6 : Address some comments #

Patch Set 7 : Make MSVC happy #

Patch Set 8 : Rebased ontop of recent parser/ast renames #

Patch Set 9 : make "destructuring assignment in parameter initializers" tests more meaty #

Patch Set 10 : Add explicit placeholder RewritableExpression for rewriting #

Total comments: 10

Patch Set 11 : Rebased #

Patch Set 12 : Make linux happy #

Patch Set 13 : Address some comments and hopefully fix build (it builds in clang here!) #

Patch Set 14 : Get rid of check #1, only use check #2 #

Patch Set 15 : Oh god it seems to work again thank goodness #

Total comments: 5

Patch Set 16 : Remove facilities for rewriting the expression multiple ways #

Total comments: 5

Patch Set 17 : Rename to RewritableAssignmentExpression + some other cleanup #

Patch Set 18 : Prevent redundant rewriting of ambiguous BindingPattern/AssignmentPatterns #

Patch Set 19 : Rebase*** oops #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1495 lines, -99 lines) Patch
M src/ast/ast.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +47 lines, -5 lines 0 comments Download
M src/ast/ast-expression-visitor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +7 lines, -0 lines 0 comments Download
M src/ast/ast-literal-reindexer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +6 lines, -0 lines 0 comments Download
M src/ast/ast-numbering.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +9 lines, -0 lines 0 comments Download
M src/ast/prettyprinter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +18 lines, -0 lines 0 comments Download
M src/bailout-reason.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +2 lines, -0 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +6 lines, -0 lines 0 comments Download
M src/compiler/ast-loop-assignment-analyzer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +6 lines, -0 lines 0 comments Download
M src/crankshaft/hydrogen.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +7 lines, -0 lines 0 comments Download
M src/crankshaft/typing.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +6 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -1 line 0 comments Download
M src/full-codegen/full-codegen.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +6 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +6 lines, -0 lines 0 comments Download
M src/messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +3 lines, -0 lines 0 comments Download
M src/parsing/expression-classifier.h View 1 2 3 4 5 6 7 9 chunks +55 lines, -3 lines 0 comments Download
M src/parsing/parser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +43 lines, -3 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +81 lines, -0 lines 0 comments Download
M src/parsing/pattern-rewriter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 11 chunks +217 lines, -42 lines 0 comments Download
M src/parsing/preparser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 36 chunks +267 lines, -43 lines 0 comments Download
M src/typing-asm.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +8 lines, -0 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +262 lines, -2 lines 0 comments Download
A test/mjsunit/harmony/destructuring-assignment.js View 1 2 3 4 5 6 7 8 1 chunk +430 lines, -0 lines 0 comments Download

Messages

Total messages: 126 (36 generated)
caitp (gmail)
PTAL when time permits This is the third try at this (following https://crrev.com/1168643005 and https://crrev.com/1301523004. ...
3 years, 8 months ago (2015-08-26 20:58:54 UTC) #1
caitp (gmail)
On 2015/08/26 20:58:54, caitp wrote: > PTAL when time permits > > This is the ...
3 years, 8 months ago (2015-08-26 21:05:57 UTC) #2
conradw
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158 src/expression-classifier.h:158: void RecordPatternError(const Scanner::Location& loc, BindingPatternErrors and AssignmentPatternErrors being separate ...
3 years, 8 months ago (2015-08-27 09:40:26 UTC) #3
caitp (gmail)
Thanks for the look https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158 src/expression-classifier.h:158: void RecordPatternError(const Scanner::Location& loc, On ...
3 years, 8 months ago (2015-08-27 15:16:01 UTC) #4
conradw
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158 src/expression-classifier.h:158: void RecordPatternError(const Scanner::Location& loc, On 2015/08/27 15:16:01, caitp wrote: ...
3 years, 8 months ago (2015-08-27 15:32:24 UTC) #5
wingo
https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158 src/expression-classifier.h:158: void RecordPatternError(const Scanner::Location& loc, On 2015/08/27 15:32:24, conradw wrote: ...
3 years, 8 months ago (2015-08-28 09:54:13 UTC) #6
conradw
On 2015/08/28 09:54:13, wingo wrote: > https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h > File src/expression-classifier.h (right): > > https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158 > ...
3 years, 7 months ago (2015-08-31 11:22:53 UTC) #7
wingo
On 2015/08/31 11:22:53, conradw wrote: > On 2015/08/28 09:54:13, wingo wrote: > > https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h > ...
3 years, 7 months ago (2015-08-31 13:13:30 UTC) #8
conradw
On 2015/08/31 13:13:30, wingo wrote: > On 2015/08/31 11:22:53, conradw wrote: > > On 2015/08/28 ...
3 years, 7 months ago (2015-08-31 13:43:34 UTC) #9
caitp (gmail)
On 2015/08/31 13:43:34, conradw wrote: > On 2015/08/31 13:13:30, wingo wrote: > > On 2015/08/31 ...
3 years, 7 months ago (2015-09-03 19:20:13 UTC) #10
wingo
Looking good. Some open questions on my side. I think the runtime bit is going ...
3 years, 7 months ago (2015-09-04 12:39:30 UTC) #11
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsing.cc#newcode7078 test/cctest/test-parsing.cc:7078: kError, NULL, 0, always_flags, arraysize(always_flags)); On 2015/09/04 12:39:30, wingo ...
3 years, 7 months ago (2015-09-04 13:08:16 UTC) #12
caitp (gmail)
On 2015/09/04 13:08:16, caitp wrote: > https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsing.cc > File test/cctest/test-parsing.cc (right): > > https://codereview.chromium.org/1309813007/diff/10001/test/cctest/test-parsing.cc#newcode7078 > ...
3 years, 7 months ago (2015-09-04 14:26:57 UTC) #13
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/10001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1309813007/diff/10001/src/flag-definitions.h#newcode416 src/flag-definitions.h:416: from rebasing, pay no mind https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h File src/preparser.h (right): ...
3 years, 7 months ago (2015-09-04 14:42:21 UTC) #14
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/30001/src/preparser.h#newcode3031 src/preparser.h:3031: PushArrowFormalsState(false); On 2015/09/04 14:42:21, caitp wrote: > This never ...
3 years, 7 months ago (2015-09-04 14:55:53 UTC) #15
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode811 src/preparser.h:811: void SetCoverInitializedNameLocation(int begin, int end) override { On 2015/09/04 ...
3 years, 7 months ago (2015-09-17 17:05:21 UTC) #16
caitp (gmail)
So the latest change involves recording in ExpressionClassifier whether a CoverInitializedName occurs, which makes the ...
3 years, 7 months ago (2015-09-24 09:09:34 UTC) #17
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/10001/src/preparser.h#newcode708 src/preparser.h:708: } On 2015/09/04 12:39:30, wingo wrote: > I don't ...
3 years, 7 months ago (2015-09-24 10:09:11 UTC) #18
caitp (gmail)
so, the big problem with eagerly rewriting destructuring assignment, is the ambiguity with arrow functions: ...
3 years, 6 months ago (2015-10-21 12:19:32 UTC) #19
caitp (gmail)
I've added an alternative implementation, but I'm not positive it's really adding much over the ...
3 years, 6 months ago (2015-10-22 17:45:42 UTC) #20
adamk
Back in the USA, and now I've lost state on this. Is this the change ...
3 years, 6 months ago (2015-10-27 19:16:58 UTC) #22
caitp (gmail)
On 2015/10/27 19:16:58, adamk wrote: > Back in the USA, and now I've lost state ...
3 years, 6 months ago (2015-10-27 19:18:09 UTC) #23
adamk
It's taking me longer than I'd like to boot up on this (hard to keep ...
3 years, 6 months ago (2015-10-27 23:04:18 UTC) #24
caitp (gmail)
Thanks --- I'll spend some time addressing these comments, but I'm still leaning more towards ...
3 years, 6 months ago (2015-10-27 23:20:05 UTC) #25
adamk
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode2477 src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true); On 2015/10/27 23:20:05, caitp wrote: ...
3 years, 6 months ago (2015-10-27 23:23:58 UTC) #26
caitp (gmail)
On 2015/10/27 23:23:58, adamk wrote: > https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h > File src/preparser.h (right): > > https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode2477 > ...
3 years, 6 months ago (2015-10-28 05:03:10 UTC) #27
caitp (gmail)
On 2015/10/28 05:03:10, caitp wrote: > On 2015/10/27 23:23:58, adamk wrote: > > https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h > ...
3 years, 6 months ago (2015-10-28 05:04:03 UTC) #28
adamk
Understanding more and more (or so I think)... https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode319 src/expression-classifier.h:319: static ...
3 years, 6 months ago (2015-10-28 19:11:41 UTC) #29
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode324 src/expression-classifier.h:324: if (b.location.beg_pos < 0 || b.location.beg_pos >= a.location.beg_pos) { ...
3 years, 6 months ago (2015-10-28 19:57:34 UTC) #30
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h File src/expression-classifier.h (right): https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode324 src/expression-classifier.h:324: if (b.location.beg_pos < 0 || b.location.beg_pos >= a.location.beg_pos) { ...
3 years, 5 months ago (2015-10-29 16:38:41 UTC) #31
adamk
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2622 src/preparser.h:2622: classifier->RecordPatternError( On 2015/10/29 16:38:41, caitp wrote: > On 2015/10/28 ...
3 years, 5 months ago (2015-10-29 23:43:21 UTC) #32
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2622 src/preparser.h:2622: classifier->RecordPatternError( On 2015/10/29 23:43:21, adamk wrote: > On 2015/10/29 ...
3 years, 5 months ago (2015-10-30 00:57:19 UTC) #33
adamk
It seems to me that supporting parsing of destructuring assignment and improving error messages for ...
3 years, 5 months ago (2015-11-02 20:46:30 UTC) #34
caitp (gmail)
just making a few notes (stealing a small portion of this CL, noticed some things ...
3 years, 5 months ago (2015-11-03 17:43:46 UTC) #35
caitp (gmail)
Okay, updated with an implementation. It's pretty messy, but it seems to work locally. Not ...
3 years, 5 months ago (2015-11-17 02:51:33 UTC) #36
caitp (gmail)
On 2015/11/17 02:51:33, caitp wrote: > Okay, updated with an implementation. > > It's pretty ...
3 years, 5 months ago (2015-11-17 03:03:20 UTC) #37
caitp (gmail)
On 2015/11/17 03:03:20, caitp wrote: > On 2015/11/17 02:51:33, caitp wrote: > > Okay, updated ...
3 years, 5 months ago (2015-11-17 04:13:17 UTC) #38
adamk
Having read through a few times, I have some high-level questions: - You mentioned on ...
3 years, 5 months ago (2015-11-20 19:49:48 UTC) #39
caitp (gmail)
On 2015/11/20 19:49:48, adamk wrote: > Having read through a few times, I have some ...
3 years, 5 months ago (2015-11-20 19:56:45 UTC) #40
adamk
On 2015/11/20 19:56:45, caitp wrote: > On 2015/11/20 19:49:48, adamk wrote: > > Having read ...
3 years, 5 months ago (2015-11-20 20:02:24 UTC) #41
caitp (gmail)
On 2015/11/20 20:02:24, adamk wrote: > On 2015/11/20 19:56:45, caitp wrote: > > On 2015/11/20 ...
3 years, 5 months ago (2015-11-20 20:05:41 UTC) #42
caitp (gmail)
On 2015/11/20 20:05:41, caitp wrote: > On 2015/11/20 20:02:24, adamk wrote: > > On 2015/11/20 ...
3 years, 5 months ago (2015-11-20 21:42:38 UTC) #43
adamk
Another source of tests for this would be the test262 tests in language/expressions/assignment/destructuring (currently all ...
3 years, 5 months ago (2015-11-20 22:42:59 UTC) #44
caitp (gmail)
I'll update the patch set Monday https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h#newcode678 src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use ...
3 years, 5 months ago (2015-11-20 23:10:25 UTC) #45
adamk
https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h#newcode678 src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use parallel compaction") On 2015/11/20 23:10:25, caitp ...
3 years, 5 months ago (2015-11-20 23:16:55 UTC) #46
caitp (gmail)
On 2015/11/20 23:16:55, adamk wrote: > https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h > File src/flag-definitions.h (right): > > https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h#newcode678 > ...
3 years, 5 months ago (2015-11-23 16:42:50 UTC) #47
caitp (gmail)
Got the test262 run passing most of the destructuring assignment tests (remaining ones that are ...
3 years, 5 months ago (2015-11-24 17:42:23 UTC) #48
caitp (gmail)
Got the test262 run passing most of the destructuring assignment tests (remaining ones that are ...
3 years, 5 months ago (2015-11-24 17:42:26 UTC) #49
caitp (gmail)
So, there are still some mixups in the AST visitors, and it would probably be ...
3 years, 5 months ago (2015-11-24 18:06:39 UTC) #50
adamk
Overall I really do prefer putting this stuff in Assignment. A few questions about other ...
3 years, 5 months ago (2015-11-24 19:52:10 UTC) #51
caitp (gmail)
Thanks for the comments --- I think the various AST visitor questions I asked are ...
3 years, 5 months ago (2015-11-24 20:26:27 UTC) #52
caitp (gmail)
+yang, do you have an idea what would cause the DCHECK failure in VerifyRecompiledCode()? It ...
3 years, 5 months ago (2015-11-24 21:37:29 UTC) #54
caitp (gmail)
On 2015/11/24 21:37:29, caitp wrote: > +yang, do you have an idea what would cause ...
3 years, 5 months ago (2015-11-25 00:04:51 UTC) #55
Yang
On 2015/11/25 00:04:51, caitp wrote: > On 2015/11/24 21:37:29, caitp wrote: > > +yang, do ...
3 years, 5 months ago (2015-11-25 01:41:13 UTC) #56
Yang
On 2015/11/25 01:41:13, Yang wrote: > On 2015/11/25 00:04:51, caitp wrote: > > On 2015/11/24 ...
3 years, 5 months ago (2015-11-25 01:44:58 UTC) #57
caitp (gmail)
On 2015/11/25 01:44:58, Yang wrote: > On 2015/11/25 01:41:13, Yang wrote: > > On 2015/11/25 ...
3 years, 5 months ago (2015-11-25 17:28:14 UTC) #58
adamk
I understand more and more each time, quite exciting. https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc File src/ast-expression-visitor.cc (right): https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc#newcode270 src/ast-expression-visitor.cc:270: ...
3 years, 5 months ago (2015-11-25 21:05:28 UTC) #80
caitp (gmail)
I'll get to these in a bit, adding some more tests first https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc File src/ast-expression-visitor.cc ...
3 years, 5 months ago (2015-11-25 21:41:52 UTC) #81
adamk
https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc File src/pattern-rewriter.cc (right): https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc#newcode276 src/pattern-rewriter.cc:276: set_context(context); On 2015/11/25 21:41:51, caitp wrote: > On 2015/11/25 ...
3 years, 5 months ago (2015-11-25 21:54:21 UTC) #82
caitp (gmail)
So, I'm still not really happy with this approach, but it is what it is. ...
3 years, 4 months ago (2015-11-30 15:47:37 UTC) #83
adamk
On 2015/11/30 15:47:37, caitp wrote: > So, I'm still not really happy with this approach, ...
3 years, 4 months ago (2015-11-30 19:20:28 UTC) #84
adamk
On 2015/11/30 19:20:28, adamk wrote: > On 2015/11/30 15:47:37, caitp wrote: > > So, I'm ...
3 years, 4 months ago (2015-11-30 19:22:05 UTC) #85
caitp (gmail)
I've added the RewritableExpression node, which I think makes this a little bit clearer. It ...
3 years, 4 months ago (2015-12-01 22:21:37 UTC) #86
adamk
I actually think this is looking pretty good. Why do you think this is fragile ...
3 years, 4 months ago (2015-12-01 22:42:57 UTC) #87
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode4549 src/parsing/parser.cc:4549: !rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) { On 2015/12/01 22:42:56, adamk wrote: > It's ...
3 years, 4 months ago (2015-12-01 22:51:00 UTC) #88
adamk
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode4549 src/parsing/parser.cc:4549: !rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) { On 2015/12/01 22:51:00, caitp wrote: > On ...
3 years, 4 months ago (2015-12-01 22:54:23 UTC) #89
caitp (gmail)
Alright, re-fixed
3 years, 4 months ago (2015-12-02 00:20:51 UTC) #90
adamk
On 2015/12/02 00:20:51, caitp wrote: > Alright, re-fixed Fixed is good, ping for re-review when ...
3 years, 4 months ago (2015-12-02 00:44:57 UTC) #91
caitp (gmail)
On 2015/12/02 00:44:57, adamk wrote: > On 2015/12/02 00:20:51, caitp wrote: > > Alright, re-fixed ...
3 years, 4 months ago (2015-12-02 01:05:45 UTC) #92
adamk
I've noted what I mean by de-generalizing. I'm all for reusable code, but I tend ...
3 years, 4 months ago (2015-12-02 01:13:13 UTC) #93
caitp (gmail)
On 2015/12/02 01:13:13, adamk wrote: > I've noted what I mean by de-generalizing. > > ...
3 years, 4 months ago (2015-12-02 01:40:31 UTC) #94
adamk
lgtm once nits are taken care of and the CL description is trimmed down/rewritten to ...
3 years, 4 months ago (2015-12-02 01:51:36 UTC) #96
caitp (gmail)
https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/1309813007/diff/710001/src/ast/ast.h#newcode3585 src/ast/ast.h:3585: RewritableExpression* NewRewritableExpression(Expression* expression) { On 2015/12/02 01:51:36, adamk wrote: ...
3 years, 4 months ago (2015-12-02 02:25:26 UTC) #97
Benedikt Meurer
LGTM on compiler, crankshaft, full-codegen and interpreter.
3 years, 4 months ago (2015-12-03 18:55:24 UTC) #102
caitp (gmail)
On 2015/12/03 18:55:24, Benedikt Meurer wrote: > LGTM on compiler, crankshaft, full-codegen and interpreter. Thanks ...
3 years, 4 months ago (2015-12-03 18:59:49 UTC) #104
rossberg
The main thing I'm confused about is why we need the rewriting queue. What prevents ...
3 years, 4 months ago (2015-12-04 14:24:21 UTC) #105
caitp (gmail)
On 2015/12/04 14:24:21, rossberg wrote: > The main thing I'm confused about is why we ...
3 years, 4 months ago (2015-12-04 14:26:09 UTC) #106
rossberg
On 2015/12/04 14:26:09, caitp wrote: > On 2015/12/04 14:24:21, rossberg wrote: > > The main ...
3 years, 4 months ago (2015-12-04 14:32:33 UTC) #107
caitp (gmail)
On 2015/12/04 14:32:33, rossberg wrote: > On 2015/12/04 14:26:09, caitp wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 14:34:43 UTC) #108
caitp (gmail)
On 2015/12/04 14:34:43, caitp wrote: > On 2015/12/04 14:32:33, rossberg wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 14:35:32 UTC) #109
rossberg
On 2015/12/04 14:35:32, caitp wrote: > On 2015/12/04 14:34:43, caitp wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 15:16:33 UTC) #110
caitp (gmail)
On 2015/12/04 15:16:33, rossberg wrote: > On 2015/12/04 14:35:32, caitp wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 15:31:49 UTC) #111
rossberg
On 2015/12/04 15:31:49, caitp wrote: > On 2015/12/04 15:16:33, rossberg wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 16:37:08 UTC) #112
caitp (gmail)
On 2015/12/04 16:37:08, rossberg wrote: > On 2015/12/04 15:31:49, caitp wrote: > > On 2015/12/04 ...
3 years, 4 months ago (2015-12-04 16:38:09 UTC) #113
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309813007/770001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309813007/770001
3 years, 4 months ago (2015-12-04 16:39:31 UTC) #117
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309813007/770001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309813007/770001
3 years, 4 months ago (2015-12-04 16:40:31 UTC) #120
commit-bot: I haz the power
Committed patchset #19 (id:770001)
3 years, 4 months ago (2015-12-04 17:20:16 UTC) #122
commit-bot: I haz the power
Patchset 19 (id:??) landed as https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95 Cr-Commit-Position: refs/heads/master@{#32623}
3 years, 4 months ago (2015-12-04 17:20:32 UTC) #124
rossberg
FWIW, Niko is looking into simplifying the rewriting mechanism as part of his patch now.
3 years, 4 months ago (2015-12-09 13:52:08 UTC) #125
adamk
3 years, 4 months ago (2015-12-09 19:25:51 UTC) #126
Message was sent while issue was closed.
On 2015/12/09 13:52:08, rossberg wrote:
> FWIW, Niko is looking into simplifying the rewriting mechanism as part of his
> patch now.

Sounds good. Can you add me to the patch (when it gets sent out, that is)?

Powered by Google App Engine
This is Rietveld 408576698