[destructuring] Implement spread binding patterns.

[destructuring] Implement spread binding patterns.

R=arv@chromium.org,rossberg@chromium.org
BUG=v8:811
LOG=N

Committed: https://crrev.com/aca4735becf173556e96c2a32cc97301c25e8093
Cr-Commit-Position: refs/heads/master@{#28522}

[Dmitry Lomov (no reviews), 2015-05-20 11:55:25]

PTAL.
I have shamelessly stolen bits and pieces of arv@'s patch here :)

I only parse array literal spreads in patterns - spreads are still disallowed in
array literal expressions.

[Dmitry Lomov (no reviews), 2015-05-20 12:13:23]

The CQ bit was checked by dslomov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-20 12:13:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151503002/1

[commit-bot: I haz the power, 2015-05-20 12:37:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 12:37:59]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-05-20 12:57:35]

lgtm

https://codereview.chromium.org/1151503002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1151503002/diff/1/src/ast.h#newcode2224
src/ast.h:2224: // Literal index is only used when Spread is renterpreted as a
pattern.
Nit: typo

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode932
src/runtime.js:932: function SpreadIntoArray(target, iterable) {
Nit: ConcatIterableToArray

[arv (Not doing code reviews), 2015-05-20 13:34:15]

LGTM

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode732
src/runtime.js:732: 
undo this line removal... (it was probably from when you moved my function)

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode932
src/runtime.js:932: function SpreadIntoArray(target, iterable) {
On 2015/05/20 12:57:35, rossberg wrote:
> Nit: ConcatIterableToArray

It is not to array... it adds it to an existing array. This is from my array
spread CL where we need to add the elements to an existing array.

For the rest destructuring we could skip passing an array and return a new
array. The only question is if we want to reuse this code or not?

[arv (Not doing code reviews), 2015-05-20 13:39:45]

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode932
src/runtime.js:932: function SpreadIntoArray(target, iterable) {
On 2015/05/20 13:34:14, arv wrote:

> For the rest destructuring we could skip passing an array and return a new
> array. The only question is if we want to reuse this code or not?

There is also $spreadIterable which almost does this

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/harmony-spr...

(it returns an InternalArray)

[rossberg, 2015-05-20 13:49:39]

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode932
src/runtime.js:932: function SpreadIntoArray(target, iterable) {
On 2015/05/20 13:34:14, arv wrote:
> On 2015/05/20 12:57:35, rossberg wrote:
> > Nit: ConcatIterableToArray
> 
> It is not to array... it adds it to an existing array. This is from my array
> spread CL where we need to add the elements to an existing array.

Hence Concat...ToArray ;)

> For the rest destructuring we could skip passing an array and return a new
> array. The only question is if we want to reuse this code or not?

It's good, as long as there is a simple way to make the array strong in strong
mode.

[arv (Not doing code reviews), 2015-05-20 13:52:25]

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h#newcode2518
src/preparser.h:2518: int literal_index =
function_state_->NextMaterializedLiteralIndex();
Is this going to be a problem in the case of array spread?

[arv (Not doing code reviews), 2015-05-20 13:58:05]

https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc#new...
src/pattern-rewriter.cc:308: // array = [];
Lets change this to not create the array here and have the runtime funciton
create the array. Then we do not need to add the literal_index to ast node.

[arv (Not doing code reviews), 2015-05-20 13:59:17]

On 2015/05/20 13:58:05, arv wrote:
> https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc
> File src/pattern-rewriter.cc (right):
> 
>
https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc#new...
> src/pattern-rewriter.cc:308: // array = [];
> Lets change this to not create the array here and have the runtime funciton
> create the array. Then we do not need to add the literal_index to ast node.

nm. That would make the array weak. The current code makes it strong since it is
tied to the current function.

[Dmitry Lomov (no reviews), 2015-05-20 14:07:19]

Quick responses to comments (I'll address others)

https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1151503002/diff/1/src/pattern-rewriter.cc#new...
src/pattern-rewriter.cc:308: // array = [];
On 2015/05/20 13:58:05, arv wrote:
> Lets change this to not create the array here and have the runtime funciton
> create the array. Then we do not need to add the literal_index to ast node.

We definitely want to create an array here, to benefit from allocation site
tracking optimizations

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h#newcode2518
src/preparser.h:2518: int literal_index =
function_state_->NextMaterializedLiteralIndex();
On 2015/05/20 13:52:25, arv wrote:
> Is this going to be a problem in the case of array spread?

I don't think so - we will just have an unused literal index.

[Dmitry Lomov (no reviews), 2015-05-20 14:22:03]

Addressed comments + do not allocate a literal index for Spread. 

PTAL

https://codereview.chromium.org/1151503002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1151503002/diff/1/src/ast.h#newcode2224
src/ast.h:2224: // Literal index is only used when Spread is renterpreted as a
pattern.
On 2015/05/20 12:57:35, rossberg wrote:
> Nit: typo

Undid this change.

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1151503002/diff/1/src/preparser.h#newcode2518
src/preparser.h:2518: int literal_index =
function_state_->NextMaterializedLiteralIndex();
On 2015/05/20 14:07:19, Dmitry Lomov (chromium) wrote:
> On 2015/05/20 13:52:25, arv wrote:
> > Is this going to be a problem in the case of array spread?
> 
> I don't think so - we will just have an unused literal index.

Scratch that - I have One Weird Trick in mind here ;)

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode732
src/runtime.js:732: 
On 2015/05/20 13:34:14, arv wrote:
> undo this line removal... (it was probably from when you moved my function)

Done.

https://codereview.chromium.org/1151503002/diff/1/src/runtime.js#newcode932
src/runtime.js:932: function SpreadIntoArray(target, iterable) {
On 2015/05/20 13:49:39, rossberg wrote:
> On 2015/05/20 13:34:14, arv wrote:
> > On 2015/05/20 12:57:35, rossberg wrote:
> > > Nit: ConcatIterableToArray
> > 
> > It is not to array... it adds it to an existing array. This is from my array
> > spread CL where we need to add the elements to an existing array.
> 
> Hence Concat...ToArray ;)
> 
> > For the rest destructuring we could skip passing an array and return a new
> > array. The only question is if we want to reuse this code or not?
> 
> It's good, as long as there is a simple way to make the array strong in strong
> mode.

Done.

[Dmitry Lomov (no reviews), 2015-05-20 14:37:46]

The CQ bit was checked by dslomov@chromium.org to run a CQ dry run

[Dmitry Lomov (no reviews), 2015-05-20 14:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1151503002/#ps20001
(title: "Comments addressed")

[commit-bot: I haz the power, 2015-05-20 14:37:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151503002/20001

[arv (Not doing code reviews), 2015-05-20 14:40:51]

LGTM

https://codereview.chromium.org/1151503002/diff/20001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):

https://codereview.chromium.org/1151503002/diff/20001/src/pattern-rewriter.cc...
src/pattern-rewriter.cc:313: // Reuse pattern's literal index - it is unused
since there is no
nice.

[Dmitry Lomov (no reviews), 2015-05-20 14:41:11]

The CQ bit was unchecked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-20 14:41:13]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-05-20 14:41:17]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-05-20 14:41:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151503002/20001

[commit-bot: I haz the power, 2015-05-20 15:06:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-20 15:06:18]

Patchset 2 (id:??) landed as
https://crrev.com/aca4735becf173556e96c2a32cc97301c25e8093
Cr-Commit-Position: refs/heads/master@{#28522}