[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

FWIW, Niko is looking into simplifying the rewriting mechanism as part of his patch now. https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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)? https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 14:35:32, caitp wrote: On 2015/12/04 14:34:43, caitp wrote: > On 2015/12/04 14:32:33, rossberg wrote: > > On 2015/12/04 14:26:09, caitp wrote: > > > On 2015/12/04 14:24:21, rossberg wrote: > > > > The main thing I'm confused about is why we need the rewriting queue. What > > > >

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 15:16:33, rossberg wrote: On 2015/12/04 14:35:32, caitp wrote: > On 2015/12/04 14:34:43, caitp wrote: > > On 2015/12/04 14:32:33, rossberg wrote: > > > On 2015/12/04 14:26:09, caitp wrote: > > > > On 2015/12/04 14:24:21, rossberg wrote: > > > > > The main thing I'm confused about

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

The main thing I'm confused about is why we need the rewriting queue. What prevents you from invoking the rewriting right at the time where you currently just queue it up? https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 14:32:33, rossberg wrote: On 2015/12/04 14:26:09, caitp wrote: > On 2015/12/04 14:24:21, rossberg wrote: > > The main thing I'm confused about is why we need the rewriting queue. What > > prevents you from invoking the rewriting right at the time where you currently > > just

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 14:34:43, caitp wrote: On 2015/12/04 14:32:33, rossberg wrote: > On 2015/12/04 14:26:09, caitp wrote: > > On 2015/12/04 14:24:21, rossberg wrote: > > > The main thing I'm confused about is why we need the rewriting queue. What > > > prevents you from invoking the rewriting

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 14:24:21, rossberg wrote: The main thing I'm confused about is why we need the rewriting queue. What prevents you from invoking the rewriting right at the time where you currently just queue it up? The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 14:26:09, caitp wrote: On 2015/12/04 14:24:21, rossberg wrote: > The main thing I'm confused about is why we need the rewriting queue. What > prevents you from invoking the rewriting right at the time where you currently > just queue it up? The main thing is the ambiguity

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Committed patchset #19 (id:770001) https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Patchset 19 (id:??) landed as https://crrev.com/b634a61d84b8793ee2939690685d4b9f7fb32b95 Cr-Commit-Position: refs/heads/master@{#32623} https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/04 15:31:49, caitp wrote: On 2015/12/04 15:16:33, rossberg wrote: > On 2015/12/04 14:35:32, caitp wrote: > > On 2015/12/04 14:34:43, caitp wrote: > > > On 2015/12/04 14:32:33, rossberg wrote: > > > > On 2015/12/04 14:26:09, caitp wrote: > > > > > On 2015/12/04 14:24:21, rossberg

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/03 18:55:24, Benedikt Meurer wrote: LGTM on compiler, crankshaft, full-codegen and interpreter. Thanks --- this makes a bit of a mess of the codebase, and I'm not sure there are too many ways around that. It would be nice to get one last round before checkin. +rossberg and maybe

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

LGTM on compiler, crankshaft, full-codegen and interpreter. https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

I've added the RewritableExpression node, which I think makes this a little bit clearer. It still looks extremely fragile to me, but more understandable maybe? https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

I actually think this is looking pretty good. Why do you think this is fragile (I mean, it at least doesn't seem any more fragile than the last two iterations)? My one big request is to de-generalize RewritableExpression (see comments in ast.h and elsewhere).

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Alright, re-fixed https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/02 00:20:51, caitp wrote: Alright, re-fixed Fixed is good, ping for re-review when you've de-generalized RewritableExpression (or when you make an argument for keeping it as-is). https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

I've noted what I mean by de-generalizing. I'm all for reusable code, but I tend to lean towards doing that once there are multiple cases. This ensures that the general code actually serves multiple use-cases well. As-is, this code looks general but in fact only handles a single case.

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/02 00:44:57, adamk wrote: On 2015/12/02 00:20:51, caitp wrote: > Alright, re-fixed Fixed is good, ping for re-review when you've de-generalized RewritableExpression (or when you make an argument for keeping it as-is). I think general is better, because there definitely are other

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

lgtm once nits are taken care of and the CL description is trimmed down/rewritten to match reality (for one, this is much more than a parsing patch now). Will also need some OWNERS in Munich, adding bmeurer who's in all the relevant files. A few nits below.

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/12/02 01:13:13, adamk wrote: I've noted what I mean by de-generalizing. I'm all for reusable code, but I tend to lean towards doing that once there are multiple cases. This ensures that the general code actually serves multiple use-cases well. As-is, this code looks general but

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

So, I'm still not really happy with this approach, but it is what it is. Maybe there's a better way to do this, like just introducing a new AST node like "RewritableExpression" or something, and wrapping the original Assignment in that if it will be rewritten (and VisitRewritableExpression

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/30 15:47:37, caitp wrote: So, I'm still not really happy with this approach, but it is what it is. Maybe there's a better way to do this, like just introducing a new AST node like "RewritableExpression" or something, and wrapping the original Assignment in that if it will be

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/30 19:20:28, adamk wrote: On 2015/11/30 15:47:37, caitp wrote: > So, I'm still not really happy with this approach, but it is what it is. > > Maybe there's a better way to do this, like just introducing a new AST node like > "RewritableExpression" or something, and wrapping the

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/25 01:44:58, Yang wrote: On 2015/11/25 01:41:13, Yang wrote: > On 2015/11/25 00:04:51, caitp wrote: > > On 2015/11/24 21:37:29, caitp wrote: > > > +yang, do you have an idea what would cause the DCHECK failure in > > > VerifyRecompiledCode()? It doesn't seem to break the code when

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Maybe scope resolved differently on second compile? Maybe there is some issue with serializing/deserializing the scope chain? Just guessing. On Wed, Nov 25, 2015, 18:28 wrote: > On 2015/11/25 01:44:58, Yang wrote: > > On 2015/11/25 01:41:13, Yang wrote: > > > On

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 (right): https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc#newcode270

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/25 01:41:13, Yang wrote: On 2015/11/25 00:04:51, caitp wrote: > On 2015/11/24 21:37:29, caitp wrote: > > +yang, do you have an idea what would cause the DCHECK failure in > > VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are > > disabled. > > More context,

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/25 00:04:51, caitp wrote: On 2015/11/24 21:37:29, caitp wrote: > +yang, do you have an idea what would cause the DCHECK failure in > VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are > disabled. More context, old_target->kind() == BUILTIN, while

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

+yang, do you have an idea what would cause the DCHECK failure in VerifyRecompiledCode()? It doesn't seem to break the code when DCHECKS are disabled. https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Thanks for the comments --- I think the various AST visitor questions I asked are probably responsible for the failing test case, so I'm going to see if I can get that fixed. https://codereview.chromium.org/1309813007/diff/410001/src/expression-classifier.h File src/expression-classifier.h

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Overall I really do prefer putting this stuff in Assignment. A few questions about other additions, but I think this is on the right track. https://codereview.chromium.org/1309813007/diff/410001/src/expression-classifier.h File src/expression-classifier.h (right):

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Got the test262 run passing most of the destructuring assignment tests (remaining ones that are still bad are all related to SetFunctionName, which isn't done yet in V8) https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Got the test262 run passing most of the destructuring assignment tests (remaining ones that are still bad are all related to SetFunctionName, which isn't done yet in V8) https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

So, there are still some mixups in the AST visitors, and it would probably be a good idea to get feedback on the design just to make sure I actually understood what you were asking for. https://codereview.chromium.org/1309813007/diff/410001/src/ast-expression-visitor.cc File

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 src/flag-definitions.h:678:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/20 20:02:24, adamk wrote: On 2015/11/20 19:56:45, caitp wrote: > On 2015/11/20 19:49:48, adamk wrote: > > Having read through a few times, I have some high-level questions: > > > > - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing > > didn't interact well

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/20 19:56:45, caitp wrote: On 2015/11/20 19:49:48, adamk wrote: > Having read through a few times, I have some high-level questions: > > - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing > didn't interact well with something else. Was that resolved?

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Having read through a few times, I have some high-level questions: - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing didn't interact well with something else. Was that resolved? Overall, do you know of any cases where this doesn't work correctly? - It seems a bit

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/20 19:49:48, adamk wrote: Having read through a few times, I have some high-level questions: - You mentioned on IRC that the keeping-track-of-assigments-to-rewrite thing didn't interact well with something else. Was that resolved? Overall, do you know of any cases where this

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/11/20 20:05:41, caitp wrote: On 2015/11/20 20:02:24, adamk wrote: > On 2015/11/20 19:56:45, caitp wrote: > > On 2015/11/20 19:49:48, adamk wrote: > > > Having read through a few times, I have some high-level questions: > > > > > > - You mentioned on IRC that the

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Another source of tests for this would be the test262 tests in language/expressions/assignment/destructuring (currently all skipped). Here are some comments on the current patch. This looks like a good direction to me; I'd really like to see stuff moved up into Assignment, though (I've noted

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Okay, updated with an implementation. It's pretty messy, but it seems to work locally. Not sure how much it can be cleaned up without a new parser https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

just making a few notes (stealing a small portion of this CL, noticed some things that aren't needed now) https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1309813007/diff/330001/src/preparser.h#newcode1023

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

It seems to me that supporting parsing of destructuring assignment and improving error messages for destructuring parse failures are separable tasks. I'd find patches for the former easier to read if they didn't also make changes to the latter at the same time.

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 ||

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 ||

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Back in the USA, and now I've lost state on this. Is this the change I ought to be reviewing? Or was there more cleanup you wanted to do on it before continuing? https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/10/27 19:16:58, adamk wrote: Back in the USA, and now I've lost state on this. Is this the change I ought to be reviewing? Or was there more cleanup you wanted to do on it before continuing? I'd look at the bits before patch set 10. Patch set 10 and beyond is mostly trying to get

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Thanks --- I'll spend some time addressing these comments, but I'm still leaning more towards the alt. implementation https://codereview.chromium.org/1309813007/diff/150001/src/ast.h File src/ast.h (right): https://codereview.chromium.org/1309813007/diff/150001/src/ast.h#newcode227

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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: On

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

It's taking me longer than I'd like to boot up on this (hard to keep the whole thing in my head), but here's some feedback in the meantime (while I read and re-read). https://codereview.chromium.org/1309813007/diff/150001/src/ast.h File src/ast.h (right):

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true); On

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 > File src/preparser.h (right): > > https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode2477 > src/preparser.h:2477:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

I've added an alternative implementation, but I'm not positive it's really adding much over the previous version. Also, I'm not 100% sure that the ambiguity issue is solved, since this is only testing parsing. The old rewriting implementation was removed, and there's some other stuff that I

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

so, the big problem with eagerly rewriting destructuring assignment, is the ambiguity with arrow functions: `({ x } = { x: 1 }) => x` -> rewriting the destructuring assignment early breaks things, because binding patterns need to be rewritten differently So there are a few ways to solve

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 see how you are going to implement the

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

So the latest change involves recording in ExpressionClassifier whether a CoverInitializedName occurs, which makes the ExpressionClassifier at least 8 bytes bigger, and likely slows down parsing a tiny bit. The reason for this, compared to the strategy in ObjectLiteralChecker, is so that the

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 12:39:30,

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

Looking good. Some open questions on my side. I think the runtime bit is going to be gnarly because e.g. we never have a loop inside an expression, currently, and that is going to throw off all kinds of things in the compiler, seems to me.

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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,

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 test/cctest/test-parsing.cc:7078:

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/08/31 13:43:34, conradw wrote: On 2015/08/31 13:13:30, wingo wrote: > 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 > > > File src/expression-classifier.h (right): >

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 src/expression-classifier.h:158: void

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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 > File src/expression-classifier.h (right): > > https://codereview.chromium.org/1309813007/diff/1/src/expression-classifier.h#newcode158

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

On 2015/08/31 13:13:30, wingo wrote: 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 > > File src/expression-classifier.h (right): > > > > >

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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,

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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,

[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)

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,