[es6] implement spread calls

[es6] implement spread calls

BUG=v8:3018
R=
LOG=N

Committed: https://crrev.com/74c381221c93253bbee513aa2539d93269303b8b
Cr-Commit-Position: refs/heads/master@{#27714}

[caitp (gmail), 2015-02-17 20:57:01]

PTAL anyone interested

It's not exactly a "desugaring", but it's the same sort of idea.

Currently, not using for-of loop in the SpreadCall helper (because it makes the
error message harder to understand).

There is an issue in debug builds hitting this DCHECK in some of the test runs
(particularly turbo-deoptimization and stress-opt)

```
# Fatal error in ../src/full-codegen.cc, line 486
# Check failed: bailout_entries_[i].id != id.
```

It doesn't seem to matter whether the new AST node is in the graph or not, which
is sort of weird. At any rate, I'll try to sort that one out.

Still missing is support for construct calls

[arv (Not doing code reviews), 2015-02-18 15:07:04]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-02-18 15:07:06]

Very nice approach. Much easier to grasp and it allows us to make good progress
as well as gives us a path for the future.

https://codereview.chromium.org/938443002/diff/20001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:2231: Visit(expr->expression());
missing todo?

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h#n...
src/flag-definitions.h:199: V(harmony_spreadcalls, "harmony spread-calls")
harmony_spread_calls maybe?

It is not clear why this is two flags?

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js#ne...
src/harmony-spread.js:9: function SpreadCall(function, thisArg, spread0) {
I'm curious how this will handle?

super(...args)  // super construct
super.method(...args)  // super method call

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js#ne...
src/harmony-spread.js:55: if (!IS_SPEC_FUNCTION(function)) {
How is NewTarget propagated?

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js#ne...
src/harmony-spread.js:95: return %ApplyConstruct(function, args, 0,
args.length);
We might need to provide one more param to %ApplyConstruct to handle NewTarget.

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc#newcode5485
src/parser.cc:5485: int length = list->length() + 3;
Please add a comment explaining "3 is the magic number".

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc#newcode5488
src/parser.cc:5488: if (function->IsProperty()) {
TODO: IsSuperReference

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc#newcode5501
src/parser.cc:5501: list->Add(factory()->NewSmiLiteral(i,
RelocInfo::kNoPosition), zone());
It is not clear to me what the list contains? I think we are storing the indexes
of the position of the spread expressions?

Maybe add a comment explaining what this looks like?

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc#newcode...
src/preparser.cc:1021: ParseArguments(&spread_pos, ok);
Does this work?

https://codereview.chromium.org/938443002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/20001/src/preparser.h#newcode2333
src/preparser.h:2333: *spread_arg_loc = spread_arg;
I'm not sure why we need to return a Location for this? Also, why is only the
first location interesting and not all of them?

Maybe a boolean is sufficient?

https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:624: static int kMaxArgc = 1000000;
Maybe define a const that is shared with Runtime_Apply?

https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:627: // If there are too many arguments,
allocate argv via malloc.
Code sharing? Maybe you need to use a macro though.

https://codereview.chromium.org/938443002/diff/20001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/20001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:229: })();
Maybe some tests with F.p.apply too?

function f(self) {
  'use strict';
  assertEquals(self, this);
}
f.call(undefined, ...[undefined]);
f.call(1, ...[1]);
var o = {};
f.call(o, ...[o]);

I would also add some tests that uses F.p.bind

[caitp (gmail), 2015-02-18 15:32:08]

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h#n...
src/flag-definitions.h:199: V(harmony_spreadcalls, "harmony spread-calls")
On 2015/02/18 15:07:05, arv wrote:
> harmony_spread_calls maybe?
> 
> It is not clear why this is two flags?

I thought it might work well to put spread calls and spread elements behind
different flags, and `harmony_spread` would implicate both of them.

It's fine to do it differently though, no strong opinion there

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/20001/src/harmony-spread.js#ne...
src/harmony-spread.js:55: if (!IS_SPEC_FUNCTION(function)) {
On 2015/02/18 15:07:06, arv wrote:
> How is NewTarget propagated?

This doesn't deal with any of the new construct protocol yet, I don't really
have much of a grasp on that stuff. Perhaps you and dslomov@ have some ideas how
it could work

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc#newcode5501
src/parser.cc:5501: list->Add(factory()->NewSmiLiteral(i,
RelocInfo::kNoPosition), zone());
On 2015/02/18 15:07:06, arv wrote:
> It is not clear to me what the list contains? I think we are storing the
indexes
> of the position of the spread expressions?
> 
> Maybe add a comment explaining what this looks like?

I tried to draw a graph of it in a comment, but it didn't add much clarity. For
reference though, it's sort of like this:

0. The function
1. The receiver
2. The index, in the list, of the index of the first spread expression (this is
where it gets confusing!)
3...N Each parameter expression passed to the call
N...M Index of each spread expression in the arguments list.

It's organized this way so that there isn't a bunch of extra array allocations
(arrays make it a bit more complicated for the preparser)

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc#newcode...
src/preparser.cc:1021: ParseArguments(&spread_pos, ok);
On 2015/02/18 15:07:06, arv wrote:
> Does this work?

No --- In the other patch I was throwing an error if it finds a spread argument
calling an intrinsic. But it might be good to make it work somehow instead

https://codereview.chromium.org/938443002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/20001/src/preparser.h#newcode2333
src/preparser.h:2333: *spread_arg_loc = spread_arg;
On 2015/02/18 15:07:06, arv wrote:
> I'm not sure why we need to return a Location for this? Also, why is only the
> first location interesting and not all of them?
> 
> Maybe a boolean is sufficient?

It's a hold-over from the other patch, where the location was used to report an
error if used in construct calls or intrinsics. So it depends whether you think
it's worth issuing an error or not, I guess

https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:627: // If there are too many arguments,
allocate argv via malloc.
On 2015/02/18 15:07:06, arv wrote:
> Code sharing? Maybe you need to use a macro though.

Eventually, there is going to be Reflect.apply() and Reflect.construct(), which
will probably be implemented as builtins respectively, and will probably work a
lot better for this.

I think we'll get rid of ApplyConstruct at that point, but if it's worth keeping
both of them around, I'll try to find a way to share more code between them.

https://codereview.chromium.org/938443002/diff/20001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/20001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:229: })();
On 2015/02/18 15:07:06, arv wrote:
> Maybe some tests with F.p.apply too?
> 
> function f(self) {
>   'use strict';
>   assertEquals(self, this);
> }
> f.call(undefined, ...[undefined]);
> f.call(1, ...[1]);
> var o = {};
> f.call(o, ...[o]);
> 
> I would also add some tests that uses F.p.bind

Will do

[arv (Not doing code reviews), 2015-02-18 15:36:54]

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/938443002/diff/20001/src/flag-definitions.h#n...
src/flag-definitions.h:199: V(harmony_spreadcalls, "harmony spread-calls")
On 2015/02/18 15:32:08, caitp wrote:
> On 2015/02/18 15:07:05, arv wrote:
> > harmony_spread_calls maybe?
> > 
> > It is not clear why this is two flags?
> 
> I thought it might work well to put spread calls and spread elements behind
> different flags, and `harmony_spread` would implicate both of them.

Good idea. I like.

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/parser.cc#newcode5501
src/parser.cc:5501: list->Add(factory()->NewSmiLiteral(i,
RelocInfo::kNoPosition), zone());
On 2015/02/18 15:32:08, caitp wrote:
> On 2015/02/18 15:07:06, arv wrote:
> > It is not clear to me what the list contains? I think we are storing the
> indexes
> > of the position of the spread expressions?
> > 
> > Maybe add a comment explaining what this looks like?
> 
> I tried to draw a graph of it in a comment, but it didn't add much clarity.
For
> reference though, it's sort of like this:
> 
> 0. The function
> 1. The receiver
> 2. The index, in the list, of the index of the first spread expression (this
is
> where it gets confusing!)
> 3...N Each parameter expression passed to the call
> N...M Index of each spread expression in the arguments list.
> 
> It's organized this way so that there isn't a bunch of extra array allocations
> (arrays make it a bit more complicated for the preparser)

I think this is a really good solution but it is not obvious what is going on
here.

(I might borrow this approach for traceur)

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/938443002/diff/20001/src/preparser.cc#newcode...
src/preparser.cc:1021: ParseArguments(&spread_pos, ok);
On 2015/02/18 15:32:08, caitp wrote:
> On 2015/02/18 15:07:06, arv wrote:
> > Does this work?
> 
> No --- In the other patch I was throwing an error if it finds a spread
argument
> calling an intrinsic. But it might be good to make it work somehow instead

Maybe a DCHECK for now then?

[Michael Starzinger, 2015-02-18 19:41:25]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-02-18 19:41:25]

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:3: // found in the LICENSE file.
What is the expected evaluation order of the following snippet? Is it "ABCXY" or
"ABXYC"? And can we have tests for it?

function* gen() { print('X'); yield 0; print('Y'); }

function a() { print('A'); }
function b() { print('B'); return gen(); }
function b() { print('C'); }
foo(a(), ...(b()), c());

[caitp (gmail), 2015-02-23 14:46:49]

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:3: // found in the LICENSE file.
On 2015/02/18 19:41:25, Michael Starzinger wrote:
> What is the expected evaluation order of the following snippet? Is it "ABCXY"
or
> "ABXYC"? And can we have tests for it?
> 
> function* gen() { print('X'); yield 0; print('Y'); }
> 
> function a() { print('A'); }
> function b() { print('B'); return gen(); }
> function b() { print('C'); }
> foo(a(), ...(b()), c());

This is a good test. I think `foo()` would be called with `[undefined, 0,
undefined]`, and "ABXYC" should be printed. But interestingly, that's not what
happens (we get the expected arguments, but "ABCXY" is logged)

That is the behaviour in FF nightly, based on http://jsfiddle.net/beumfrrm/
foo() would be called with arguments `[undefined, 0, undefined]`, and "ABXYC" is
printed. At least I think that's right :> adding test.

Adding the test and investigating that

[arv (Not doing code reviews), 2015-02-23 15:06:53]

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:3: // found in the LICENSE file.
On 2015/02/23 14:46:49, caitp wrote:
> On 2015/02/18 19:41:25, Michael Starzinger wrote:
> > What is the expected evaluation order of the following snippet? Is it
"ABCXY"
> or
> > "ABXYC"? And can we have tests for it?
> > 
> > function* gen() { print('X'); yield 0; print('Y'); }
> > 
> > function a() { print('A'); }
> > function b() { print('B'); return gen(); }
> > function b() { print('C'); }
> > foo(a(), ...(b()), c());
> 
> This is a good test. I think `foo()` would be called with `[undefined, 0,
> undefined]`, and "ABXYC" should be printed. But interestingly, that's not what
> happens (we get the expected arguments, but "ABCXY" is logged)
> 
> That is the behaviour in FF nightly, based on http://jsfiddle.net/beumfrrm/
> foo() would be called with arguments `[undefined, 0, undefined]`, and "ABXYC"
is
> printed. At least I think that's right :> adding test.
> 
> Adding the test and investigating that

The spec is clear on this (this one is easier than usual).

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-argument-lists-runti...

The correct behavior is

ABXYC and [undefinded, 0, undefined]

[caitp (gmail), 2015-02-23 15:09:28]

On 2015/02/23 15:06:53, arv wrote:
>
https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
> File test/mjsunit/harmony/spread-call.js (right):
> 
>
https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
> test/mjsunit/harmony/spread-call.js:3: // found in the LICENSE file.
> On 2015/02/23 14:46:49, caitp wrote:
> > On 2015/02/18 19:41:25, Michael Starzinger wrote:
> > > What is the expected evaluation order of the following snippet? Is it
> "ABCXY"
> > or
> > > "ABXYC"? And can we have tests for it?
> > > 
> > > function* gen() { print('X'); yield 0; print('Y'); }
> > > 
> > > function a() { print('A'); }
> > > function b() { print('B'); return gen(); }
> > > function b() { print('C'); }
> > > foo(a(), ...(b()), c());
> > 
> > This is a good test. I think `foo()` would be called with `[undefined, 0,
> > undefined]`, and "ABXYC" should be printed. But interestingly, that's not
what
> > happens (we get the expected arguments, but "ABCXY" is logged)
> > 
> > That is the behaviour in FF nightly, based on http://jsfiddle.net/beumfrrm/
> > foo() would be called with arguments `[undefined, 0, undefined]`, and
"ABXYC"
> is
> > printed. At least I think that's right :> adding test.
> > 
> > Adding the test and investigating that
> 
> The spec is clear on this (this one is easier than usual).
> 
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-argument-lists-runti...
> 
> The correct behavior is
> 
> ABXYC and [undefinded, 0, undefined]

What I mean is, investigating why this isn't what actually happens. The spead
runtime method might be interfering with the normal rules for resuming
generators

[caitp (gmail), 2015-02-23 17:24:42]

On 2015/02/23 15:09:28, caitp wrote:
> On 2015/02/23 15:06:53, arv wrote:
> >
>
https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
> > File test/mjsunit/harmony/spread-call.js (right):
> > 
> >
>
https://codereview.chromium.org/938443002/diff/40001/test/mjsunit/harmony/spr...
> > test/mjsunit/harmony/spread-call.js:3: // found in the LICENSE file.
> > On 2015/02/23 14:46:49, caitp wrote:
> > > On 2015/02/18 19:41:25, Michael Starzinger wrote:
> > > > What is the expected evaluation order of the following snippet? Is it
> > "ABCXY"
> > > or
> > > > "ABXYC"? And can we have tests for it?
> > > > 
> > > > function* gen() { print('X'); yield 0; print('Y'); }
> > > > 
> > > > function a() { print('A'); }
> > > > function b() { print('B'); return gen(); }
> > > > function b() { print('C'); }
> > > > foo(a(), ...(b()), c());
> > > 
> > > This is a good test. I think `foo()` would be called with `[undefined, 0,
> > > undefined]`, and "ABXYC" should be printed. But interestingly, that's not
> what
> > > happens (we get the expected arguments, but "ABCXY" is logged)
> > > 
> > > That is the behaviour in FF nightly, based on
http://jsfiddle.net/beumfrrm/
> > > foo() would be called with arguments `[undefined, 0, undefined]`, and
> "ABXYC"
> > is
> > > printed. At least I think that's right :> adding test.
> > > 
> > > Adding the test and investigating that
> > 
> > The spec is clear on this (this one is easier than usual).
> > 
> >
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-argument-lists-runti...
> > 
> > The correct behavior is
> > 
> > ABXYC and [undefinded, 0, undefined]
> 
> What I mean is, investigating why this isn't what actually happens. The spead
> runtime method might be interfering with the normal rules for resuming
> generators

whew, tried some things, eagerly iterating works (thanks Erik), also simplified
harmony-spread a bit.

Some changes:

- Runtime_Apply optionally takes 3 or 5 arguments (adding the offset/argc
arguments made things more complicated). Should there be 2 different versions of
the function instead?
- Runtime_Apply will check if function is callable, and throw an error if not.
Otherwise we were getting illegal instruction errors rather than a more sensible
type error which explains the problem.

[arv (Not doing code reviews), 2015-02-23 17:55:56]

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js#ne...
src/harmony-spread.js:12: var array = (new
InternalArray()).concat(%_Arguments(0));
Why not Push. concat creates a new array so we create one too many.

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js#ne...
src/harmony-spread.js:15: array = array.concat(%_Arguments(i));
push here as well

or roll your own loop. It is usually faster but I'm not sure how much cruft
internal array push has.

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js#ne...
src/harmony-spread.js:27: var iterator = GetIterator(collection, iterable);
I'm on a mission to replace all of these with for-of since for-of will be able
to correctly handle the abrubt completion in the future so we don't have to
handle that all over the code base.

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5497
src/parser.cc:5497: int i = 0;
This could use a comment explaining what it desugars to.

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5508
src/parser.cc:5508: args->Add(factory()->NewArrayLiteral(unspread,
literal_index,
Will this be observable if someone overrides Array.prototoype[Symbol.iterator]?

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:578: DCHECK(args.length() == 5 || args.length()
== 3);
I think it would be cleaner to have to functions.

[rossberg, 2015-02-24 16:22:46]

rossberg@chromium.org changed reviewers:
+ rossberg@chromium.org

[rossberg, 2015-02-24 16:22:47]

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1619:
EMPTY_NATIVE_FUNCTIONS_FOR_FEATURE(harmony_spreadcalls)
Why do we need a separate flag?

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/80001/src/harmony-spread.js#ne...
src/harmony-spread.js:12: var array = (new
InternalArray()).concat(%_Arguments(0));
On 2015/02/23 17:55:56, arv wrote:
> Why not Push. concat creates a new array so we create one too many.

+1, use push

https://codereview.chromium.org/938443002/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/80001/src/preparser.h#newcode2320
src/preparser.h:2320: Scanner::Location* spread_arg_loc, bool* ok) {
Maybe name this first_spread_arg_loc to clarify that it points to the first of
possibly many spread args.

https://codereview.chromium.org/938443002/diff/80001/src/preparser.h#newcode2349
src/preparser.h:2349: unspread_sequences_count++;
Something seems wrong with the was_unspread logic. AFAICS, this only increments
unspread_sequences_count at most once.

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:578: DCHECK(args.length() == 5 || args.length()
== 3);
On 2015/02/23 17:55:56, arv wrote:
> I think it would be cleaner to have to functions.

+1

https://codereview.chromium.org/938443002/diff/80001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/938443002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:4993: "...'123', ...'456'",
Have some more tests with spreads interleaved with non-spreads.

https://codereview.chromium.org/938443002/diff/80001/test/mjsunit/harmony/spr...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/80001/test/mjsunit/harmony/spr...
test/mjsunit/harmony/spread-call.js:247: assertEquals("ABXYC", log);
Can you extend this test to have several spreads and non-spreads mixed?

[caitp (gmail), 2015-02-25 00:00:25]

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1619:
EMPTY_NATIVE_FUNCTIONS_FOR_FEATURE(harmony_spreadcalls)
On 2015/02/24 16:22:46, rossberg wrote:
> Why do we need a separate flag?

See the answer I gave to Erik --- If spread-calls and spread-elements are behind
different flags,  it seemed nice to have a third flag which would implicate both
of them (also, it would be convenient to put any self-hosted runtime support for
both in one script).

If that's not cool, I'll just get rid of it

https://codereview.chromium.org/938443002/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/80001/src/preparser.h#newcode2349
src/preparser.h:2349: unspread_sequences_count++;
On 2015/02/24 16:22:47, rossberg wrote:
> Something seems wrong with the was_unspread logic. AFAICS, this only
increments
> unspread_sequences_count at most once.

Yeah, it was broken. Thanks for spotting that

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/runtime/runtime-func...
src/runtime/runtime-function.cc:578: DCHECK(args.length() == 5 || args.length()
== 3);
On 2015/02/24 16:22:47, rossberg wrote:
> On 2015/02/23 17:55:56, arv wrote:
> > I think it would be cleaner to have to functions.
> 
> +1

done

[rossberg, 2015-02-25 14:28:19]

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:1619:
EMPTY_NATIVE_FUNCTIONS_FOR_FEATURE(harmony_spreadcalls)
On 2015/02/25 00:00:24, caitp wrote:
> On 2015/02/24 16:22:46, rossberg wrote:
> > Why do we need a separate flag?
> 
> See the answer I gave to Erik --- If spread-calls and spread-elements are
behind
> different flags,  it seemed nice to have a third flag which would implicate
both
> of them (also, it would be convenient to put any self-hosted runtime support
for
> both in one script).
> 
> If that's not cool, I'll just get rid of it

Well, since they currently imply each other, the separation doesn't really
exist. :)  But yeah, I don't think we need feature subset flags.

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5497
src/parser.cc:5497: int i = 0;
On 2015/02/23 17:55:56, arv wrote:
> This could use a comment explaining what it desugars to.

+1

https://codereview.chromium.org/938443002/diff/160001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/938443002/diff/160001/src/flag-definitions.h#...
src/flag-definitions.h:199: V(harmony_spreadcalls, "harmony spread-calls")
Nit: spread_calls

https://codereview.chromium.org/938443002/diff/160001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/160001/src/preparser.h#newcode...
src/preparser.h:2358: int unspread_sequences_count = 0;
A brief comment explaining what this actually counts would be useful. I gather
it's the number of non-empty sequences of non-spread arguments?

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:641: // TODO(caitp): Support larger element
indexes (up to 2^53-1).
Ah, no. We likely won't, ever, at least not for calls -- you will never have a
stack that large anyway.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:643: ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate,
val,
I think it's more adequate to raise an exception here. RangeError or OOM.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:659: if (!fun->IsJSFunction()) {
Why is this needed? Isn't that already taken care of by Execution::Call?

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:682: if (!fun->IsJSFunction()) {
In a similar vein, this logic belongs into Execution::New.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h#n...
src/runtime/runtime.h:62: F(ApplyCall, 3, 1)                           \
How about slightly more descriptive names? E.g. ApplyAll and ApplyConstructAll.

[caitp (gmail), 2015-02-25 14:49:09]

Thanks

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5497
src/parser.cc:5497: int i = 0;
On 2015/02/25 14:28:18, rossberg wrote:
> On 2015/02/23 17:55:56, arv wrote:
> > This could use a comment explaining what it desugars to.
> 
> +1

Acknowledged.

https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5508
src/parser.cc:5508: args->Add(factory()->NewArrayLiteral(unspread,
literal_index,
On 2015/02/23 17:55:56, arv wrote:
> Will this be observable if someone overrides
Array.prototoype[Symbol.iterator]?

The unspread array literals aren't iterated via iterator protocol, so no. Could
be observable if the length property were defined though.

https://codereview.chromium.org/938443002/diff/160001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/938443002/diff/160001/src/preparser.h#newcode...
src/preparser.h:2358: int unspread_sequences_count = 0;
On 2015/02/25 14:28:18, rossberg wrote:
> A brief comment explaining what this actually counts would be useful. I gather
> it's the number of non-empty sequences of non-spread arguments?

That's right --- will add a comment

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:641: // TODO(caitp): Support larger element
indexes (up to 2^53-1).
On 2015/02/25 14:28:19, rossberg wrote:
> Ah, no. We likely won't, ever, at least not for calls -- you will never have a
> stack that large anyway.

sorry, that's copy/pasted from similar code I wrote in ArrayConcat, where the
"length" property is spec'd to support up to 2^53-1. I agree the comment doesn't
really apply here.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:643: ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate,
val,
On 2015/02/25 14:28:19, rossberg wrote:
> I think it's more adequate to raise an exception here. RangeError or OOM.

The only reason ToLength() would throw is if the value is an object with a
custom toString() or valueOf() which throws, iirc. So, it wouldn't really be
representative of OOM or RangeError

There isn't really a good reason to use ToLength() here anyways, though. The
arguments object should always be a JSArray, so I guess I'll just remove the
else-case here.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:682: if (!fun->IsJSFunction()) {
On 2015/02/25 14:28:19, rossberg wrote:
> In a similar vein, this logic belongs into Execution::New.

The problem with letting Execution::New() and Execution::Call() do it, is you
end up with an illegal instruction if they don't find something callable.

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h#n...
src/runtime/runtime.h:62: F(ApplyCall, 3, 1)                           \
On 2015/02/25 14:28:19, rossberg wrote:
> How about slightly more descriptive names? E.g. ApplyAll and
ApplyConstructAll.

Acknowledged.

[caitp (gmail), 2015-02-25 16:02:52]

Patchset #10 (id:180001) has been deleted

[caitp (gmail), 2015-03-23 21:46:33]

On 2015/02/25 14:49:09, caitp wrote:
> Thanks
> 
> https://codereview.chromium.org/938443002/diff/80001/src/parser.cc
> File src/parser.cc (right):
> 
> https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5497
> src/parser.cc:5497: int i = 0;
> On 2015/02/25 14:28:18, rossberg wrote:
> > On 2015/02/23 17:55:56, arv wrote:
> > > This could use a comment explaining what it desugars to.
> > 
> > +1
> 
> Acknowledged.
> 
> https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5508
> src/parser.cc:5508: args->Add(factory()->NewArrayLiteral(unspread,
> literal_index,
> On 2015/02/23 17:55:56, arv wrote:
> > Will this be observable if someone overrides
> Array.prototoype[Symbol.iterator]?
> 
> The unspread array literals aren't iterated via iterator protocol, so no.
Could
> be observable if the length property were defined though.
> 
> https://codereview.chromium.org/938443002/diff/160001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/preparser.h#newcode...
> src/preparser.h:2358: int unspread_sequences_count = 0;
> On 2015/02/25 14:28:18, rossberg wrote:
> > A brief comment explaining what this actually counts would be useful. I
gather
> > it's the number of non-empty sequences of non-spread arguments?
> 
> That's right --- will add a comment
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
> File src/runtime/runtime-function.cc (right):
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
> src/runtime/runtime-function.cc:641: // TODO(caitp): Support larger element
> indexes (up to 2^53-1).
> On 2015/02/25 14:28:19, rossberg wrote:
> > Ah, no. We likely won't, ever, at least not for calls -- you will never have
a
> > stack that large anyway.
> 
> sorry, that's copy/pasted from similar code I wrote in ArrayConcat, where the
> "length" property is spec'd to support up to 2^53-1. I agree the comment
doesn't
> really apply here.
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
> src/runtime/runtime-function.cc:643: ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate,
> val,
> On 2015/02/25 14:28:19, rossberg wrote:
> > I think it's more adequate to raise an exception here. RangeError or OOM.
> 
> The only reason ToLength() would throw is if the value is an object with a
> custom toString() or valueOf() which throws, iirc. So, it wouldn't really be
> representative of OOM or RangeError
> 
> There isn't really a good reason to use ToLength() here anyways, though. The
> arguments object should always be a JSArray, so I guess I'll just remove the
> else-case here.
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-fun...
> src/runtime/runtime-function.cc:682: if (!fun->IsJSFunction()) {
> On 2015/02/25 14:28:19, rossberg wrote:
> > In a similar vein, this logic belongs into Execution::New.
> 
> The problem with letting Execution::New() and Execution::Call() do it, is you
> end up with an illegal instruction if they don't find something callable.
> 
> https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h
> File src/runtime/runtime.h (right):
> 
>
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h#n...
> src/runtime/runtime.h:62: F(ApplyCall, 3, 1)                           \
> On 2015/02/25 14:28:19, rossberg wrote:
> > How about slightly more descriptive names? E.g. ApplyAll and
> ApplyConstructAll.
> 
> Acknowledged.

I've changed this to use ReflectApply / ReflectConstruct rather than the new
runtime functions (which can be deleted if that's cool) --- it means newTarget
stuff works correctly-ish

[caitp (gmail), 2015-03-23 22:14:25]

caitpotter88@gmail.com changed reviewers:
+ dslomov@chromium.org

[caitp (gmail), 2015-03-23 22:30:27]

Patchset #15 (id:300001) has been deleted

[arv (Not doing code reviews), 2015-03-30 22:19:50]

Very nice

https://codereview.chromium.org/938443002/diff/280001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/280001/src/harmony-spread.js#n...
src/harmony-spread.js:32: if (IS_NULL_OR_UNDEFINED(collection)) {
Isn't this is already tested by for-of?

https://codereview.chromium.org/938443002/diff/280001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/preparser.cc#newcod...
src/preparser.cc:1049: // TODO(caitp): support intrinsics
I don't think we need to support that.

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:4616: if (expr->IsSuperCall()) {
This is a bit strange/hacky. I assume this comes from

  super(...args)

which we desugar to

  %_ReflectConstruct(args, new.target)

?

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
File test/mjsunit/harmony/spread-call-new-class.js (right):

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call-new-class.js:5: // Flags: --harmony-spreadcalls
--harmony-sloppy --harmony-classes
--harmony-classes can be removed

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call-new-class.js:20: super(...args);
Awesome!

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call-new-class.js:20: super(...args);
Maybe also add a case with `super(x, ...y)`

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
2015

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:19: assertEquals(3, countArgs(..."tes"));
No need to test this many different strings.

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:135: assertEquals(3,
O.countArgs(...(function*(){ yield 1; yield 2; yield 3; })()));
long line

[caitp (gmail), 2015-03-30 22:38:12]

https://codereview.chromium.org/938443002/diff/280001/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/280001/src/harmony-spread.js#n...
src/harmony-spread.js:32: if (IS_NULL_OR_UNDEFINED(collection)) {
On 2015/03/30 22:19:49, arv wrote:
> Isn't this is already tested by for-of?

at the time I wrote it, it was a reference error trying to get Symbol.iterator
from null/undefined --- it might be a better error now

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:4616: if (expr->IsSuperCall()) {
On 2015/03/30 22:19:49, arv wrote:
> This is a bit strange/hacky. I assume this comes from
> 
>   super(...args)
> 
> which we desugar to
> 
>   %_ReflectConstruct(args, new.target)
> 
> ?

it's kind of a hack, yeah... but, without it, `this` binding doesn't get
initialized for supercalls with spread

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
File test/mjsunit/harmony/spread-call-new-class.js (right):

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call-new-class.js:20: super(...args);
On 2015/03/30 22:19:49, arv wrote:
> Maybe also add a case with `super(x, ...y)`

Acknowledged.

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
File test/mjsunit/harmony/spread-call.js (right):

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
On 2015/03/30 22:19:49, arv wrote:
> 2015

Acknowledged.

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:19: assertEquals(3, countArgs(..."tes"));
On 2015/03/30 22:19:49, arv wrote:
> No need to test this many different strings.

I'll get rid of a few redundant cases

https://codereview.chromium.org/938443002/diff/280001/test/mjsunit/harmony/sp...
test/mjsunit/harmony/spread-call.js:135: assertEquals(3,
O.countArgs(...(function*(){ yield 1; yield 2; yield 3; })()));
On 2015/03/30 22:19:49, arv wrote:
> long line

Acknowledged.

[Dmitry Lomov (no reviews), 2015-03-31 10:09:43]

My biggest concern is IsSuperFlag on CallRuntime node.
Can you also provide a high-level explanation of desugaring, i.e. what:
  f(x,y,...args);
  new f(x,y,...args)
  super(x,y,...args)
desugar into in the AST

https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2422: Visit(expr->expression());
Bailout instead of visiting silently

https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h#newc...
src/full-codegen.h:375: if (allow_super && expr->IsSuperReference()) {
Do not do this. 
Instead, call EmitLoadSuperConstructor() explicitly.
Meaning of 'super' is different in different contexts.

https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc#newcode...
src/hydrogen.cc:11055: Visit(expr->expression());
Bailout instead of visiting.

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:4616: if (expr->IsSuperCall()) {
On 2015/03/30 22:38:12, caitp wrote:
> On 2015/03/30 22:19:49, arv wrote:
> > This is a bit strange/hacky. I assume this comes from
> > 
> >   super(...args)
> > 
> > which we desugar to
> > 
> >   %_ReflectConstruct(args, new.target)
> > 
> > ?
> 
> it's kind of a hack, yeah... but, without it, `this` binding doesn't get
> initialized for supercalls with spread

IsSuperCall flag on CallRuntime node is super-hacky. Can we have a different
node for spread calls?

[caitp (gmail), 2015-03-31 12:28:40]

On 2015/03/31 10:09:43, Dmitry Lomov (chromium) wrote:
> My biggest concern is IsSuperFlag on CallRuntime node.
> Can you also provide a high-level explanation of desugaring, i.e. what:
>   f(x,y,...args);
>   new f(x,y,...args)
>   super(x,y,...args)
> desugar into in the AST


f(x, y, ...args) -> ReflectApply(f, undefined, SpreadArguments([x, y],
SpreadIterable(args))
new f(x, y, ...args) -> ReflectConstruct(f, SpreadArguments([x, y],
SpreadIterable(args))
super(x, y, ...args) -> this = ReflectConstruct(GetPrototype(%CurrentFunction%),
SpreadArguments([x, y], SpreadIterable(args)), %NewTarget%)

> 
>
https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
> src/compiler/ast-graph-builder.cc:2422: Visit(expr->expression());
> Bailout instead of visiting silently
> 
> https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h
> File src/full-codegen.h (right):
> 
>
https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h#newc...
> src/full-codegen.h:375: if (allow_super && expr->IsSuperReference()) {
> Do not do this. 
> Instead, call EmitLoadSuperConstructor() explicitly.
> Meaning of 'super' is different in different contexts.
> 
> https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc#newcode...
> src/hydrogen.cc:11055: Visit(expr->expression());
> Bailout instead of visiting.
> 
>
https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
> File src/x64/full-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
> src/x64/full-codegen-x64.cc:4616: if (expr->IsSuperCall()) {
> On 2015/03/30 22:38:12, caitp wrote:
> > On 2015/03/30 22:19:49, arv wrote:
> > > This is a bit strange/hacky. I assume this comes from
> > > 
> > >   super(...args)
> > > 
> > > which we desugar to
> > > 
> > >   %_ReflectConstruct(args, new.target)
> > > 
> > > ?
> > 
> > it's kind of a hack, yeah... but, without it, `this` binding doesn't get
> > initialized for supercalls with spread
> 
> IsSuperCall flag on CallRuntime node is super-hacky. Can we have a different
> node for spread calls?

It's hacky --- but, having another node would have to do almost identical work
to CallRuntime, and would be used very little. If that seems worthwhile, then
that can be done instead

The reason this is done is 2 things: ordinarily, CallRuntime will not be able to
deal with `super()` as a function (because visiting that node throws, so
EmitLoadSuperConstructor() is needed instead of just visiting the node).
Secondly, initializing the `this` binding is also needed for super calls. A flag
seemed like the best ay to do this while not rewriting all the relevant
CallRuntime code

[caitp (gmail), 2015-03-31 12:45:16]

On 2015/03/31 12:28:40, caitp wrote:
> On 2015/03/31 10:09:43, Dmitry Lomov (chromium) wrote:
> > My biggest concern is IsSuperFlag on CallRuntime node.
> > Can you also provide a high-level explanation of desugaring, i.e. what:
> >   f(x,y,...args);
> >   new f(x,y,...args)
> >   super(x,y,...args)
> > desugar into in the AST
> 
> 
> f(x, y, ...args) -> ReflectApply(f, undefined, SpreadArguments([x, y],
> SpreadIterable(args))
> new f(x, y, ...args) -> ReflectConstruct(f, SpreadArguments([x, y],
> SpreadIterable(args))
> super(x, y, ...args) -> this =
ReflectConstruct(GetPrototype(%CurrentFunction%),
> SpreadArguments([x, y], SpreadIterable(args)), %NewTarget%)

To expand on this, it's very close to what Traceur is doing --- sequences of 1
or more non-spread arguments are expressed in array literals (done in the
parser), each spread argument is spread into an array (SpreadIterable does
this), and these arrays are joined into a single array (in SpreadArguments).

The array is passed as the arguments array, to either Reflect.apply, or
Reflect.construct, For Reflect.apply, the `this` value is either undefined, or
the object containing whatever property is being called.

For Reflect.construct, if the call is a super() call, the newTarget argument is
the a reference to the scope's new target binding

[Dmitry Lomov (no reviews), 2015-03-31 12:59:30]

On 2015/03/31 12:28:40, caitp wrote:
> On 2015/03/31 10:09:43, Dmitry Lomov (chromium) wrote:
> > My biggest concern is IsSuperFlag on CallRuntime node.
> > Can you also provide a high-level explanation of desugaring, i.e. what:
> >   f(x,y,...args);
> >   new f(x,y,...args)
> >   super(x,y,...args)
> > desugar into in the AST
> 
> 
> f(x, y, ...args) -> ReflectApply(f, undefined, SpreadArguments([x, y],
> SpreadIterable(args))
> new f(x, y, ...args) -> ReflectConstruct(f, SpreadArguments([x, y],
> SpreadIterable(args))
> super(x, y, ...args) -> this =
ReflectConstruct(GetPrototype(%CurrentFunction%),
> SpreadArguments([x, y], SpreadIterable(args)), %NewTarget%)
> 
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
> > File src/compiler/ast-graph-builder.cc (right):
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/compiler/ast-graph-...
> > src/compiler/ast-graph-builder.cc:2422: Visit(expr->expression());
> > Bailout instead of visiting silently
> > 
> > https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h
> > File src/full-codegen.h (right):
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/full-codegen.h#newc...
> > src/full-codegen.h:375: if (allow_super && expr->IsSuperReference()) {
> > Do not do this. 
> > Instead, call EmitLoadSuperConstructor() explicitly.
> > Meaning of 'super' is different in different contexts.
> > 
> > https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc
> > File src/hydrogen.cc (right):
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/hydrogen.cc#newcode...
> > src/hydrogen.cc:11055: Visit(expr->expression());
> > Bailout instead of visiting.
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
> > File src/x64/full-codegen-x64.cc (right):
> > 
> >
>
https://codereview.chromium.org/938443002/diff/280001/src/x64/full-codegen-x6...
> > src/x64/full-codegen-x64.cc:4616: if (expr->IsSuperCall()) {
> > On 2015/03/30 22:38:12, caitp wrote:
> > > On 2015/03/30 22:19:49, arv wrote:
> > > > This is a bit strange/hacky. I assume this comes from
> > > > 
> > > >   super(...args)
> > > > 
> > > > which we desugar to
> > > > 
> > > >   %_ReflectConstruct(args, new.target)
> > > > 
> > > > ?
> > > 
> > > it's kind of a hack, yeah... but, without it, `this` binding doesn't get
> > > initialized for supercalls with spread
> > 
> > IsSuperCall flag on CallRuntime node is super-hacky. Can we have a different
> > node for spread calls?
> 
> It's hacky --- but, having another node would have to do almost identical work
> to CallRuntime, and would be used very little. If that seems worthwhile, then
> that can be done instead
> 
> The reason this is done is 2 things: ordinarily, CallRuntime will not be able
to
> deal with `super()` as a function (because visiting that node throws, so
> EmitLoadSuperConstructor() is needed instead of just visiting the node).
> Secondly, initializing the `this` binding is also needed for super calls. A
flag
> seemed like the best ay to do this while not rewriting all the relevant
> CallRuntime code

Let us have special intrinsics %_CallWithSpread and %_ConstructWithSpread
instead of flags on a general CallRuntime node (see DefaultConstructorCallSuper
for inspiration).

[caitp (gmail), 2015-03-31 15:37:18]

On 2015/03/31 12:59:30, Dmitry Lomov (chromium) wrote:
> Let us have special intrinsics %_CallWithSpread and %_ConstructWithSpread
> instead of flags on a general CallRuntime node (see
DefaultConstructorCallSuper
> for inspiration).

done --- (Super() is the only case that needed special behaviour).

Unfortunately, one of the changes in the last patchset caused some issues in
debug builds (a bailout_id gets reused, and fails a DCHECK in full-codegen
l513). I'm not totally sure why the bailout id is reused, but it's probably
because of some UNREACHABLES() in VisitSpread() that shouldn't be there... I'll
try to fix that up in a sec

[caitp (gmail), 2015-03-31 16:01:10]

On 2015/03/31 15:37:18, caitp wrote:
> On 2015/03/31 12:59:30, Dmitry Lomov (chromium) wrote:
> > Let us have special intrinsics %_CallWithSpread and %_ConstructWithSpread
> > instead of flags on a general CallRuntime node (see
> DefaultConstructorCallSuper
> > for inspiration).
> 
> done --- (Super() is the only case that needed special behaviour).
> 
> Unfortunately, one of the changes in the last patchset caused some issues in
> debug builds (a bailout_id gets reused, and fails a DCHECK in full-codegen
> l513). I'm not totally sure why the bailout id is reused, but it's probably
> because of some UNREACHABLES() in VisitSpread() that shouldn't be there...
I'll
> try to fix that up in a sec

hmm no, that's not what it is --- seems to be an issue with the way spread
method calls are desugared

[caitp (gmail), 2015-03-31 16:50:18]

Patchset #16 (id:340001) has been deleted

[caitp (gmail), 2015-03-31 17:05:21]

On 2015/03/31 16:01:10, caitp wrote:
> On 2015/03/31 15:37:18, caitp wrote:
> > On 2015/03/31 12:59:30, Dmitry Lomov (chromium) wrote:
> > > Let us have special intrinsics %_CallWithSpread and %_ConstructWithSpread
> > > instead of flags on a general CallRuntime node (see
> > DefaultConstructorCallSuper
> > > for inspiration).
> > 
> > done --- (Super() is the only case that needed special behaviour).
> > 
> > Unfortunately, one of the changes in the last patchset caused some issues in
> > debug builds (a bailout_id gets reused, and fails a DCHECK in full-codegen
> > l513). I'm not totally sure why the bailout id is reused, but it's probably
> > because of some UNREACHABLES() in VisitSpread() that shouldn't be there...
> I'll
> > try to fix that up in a sec
> 
> hmm no, that's not what it is --- seems to be an issue with the way spread
> method calls are desugared

Okay --- fixed that little issue, and nits addressed

[caitp (gmail), 2015-03-31 17:14:09]

Patchset #17 (id:380001) has been deleted

[arv (Not doing code reviews), 2015-03-31 18:54:32]

LGTM

Please wait for dmitry or mstarzinger before CQing.

[caitp (gmail), 2015-03-31 19:00:27]

On 2015/03/31 18:54:32, arv wrote:
> LGTM
> 
> Please wait for dmitry or mstarzinger before CQing.

thanks --- no CQing until after branch and after ports are written anyways (need
an lgtm on the codegen)

[Dmitry Lomov (no reviews), 2015-04-02 07:53:33]

looks good, go ahead with platform ports!

https://codereview.chromium.org/938443002/diff/380002/src/flag-definitions.h
File src/flag-definitions.h (left):

https://codereview.chromium.org/938443002/diff/380002/src/flag-definitions.h#...
src/flag-definitions.h:237: 
nit: restore this empty line

https://codereview.chromium.org/938443002/diff/380002/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/938443002/diff/380002/src/flag-definitions.h#...
src/flag-definitions.h:184: 
nit: stray empty line

https://codereview.chromium.org/938443002/diff/380002/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/380002/src/harmony-spread.js#n...
src/harmony-spread.js:14: if (SpreadArguments) return;
Why this 'if' is needed?

https://codereview.chromium.org/938443002/diff/380002/src/harmony-spread.js#n...
src/harmony-spread.js:16: SpreadArguments = function spreadArguments() {
These functions will be eagerly compiled at snapshot-building time. To avoid
this, please move them outside the SetupSpreadHelper.

[Dmitry Lomov (no reviews), 2015-04-02 07:54:20]

https://codereview.chromium.org/938443002/diff/380002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/938443002/diff/380002/src/hydrogen.cc#newcode...
src/hydrogen.cc:11031: void HOptimizedGraphBuilder::VisitSpread(Spread* expr) {
UNREACHABLE(); }
One thing I am missing: why this is never hit?

[caitp (gmail), 2015-04-02 13:03:51]

https://codereview.chromium.org/938443002/diff/380002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/938443002/diff/380002/src/hydrogen.cc#newcode...
src/hydrogen.cc:11031: void HOptimizedGraphBuilder::VisitSpread(Spread* expr) {
UNREACHABLE(); }
On 2015/04/02 07:54:20, Dmitry Lomov (chromium) wrote:
> One thing I am missing: why this is never hit?

the Spread nodes are removed from the graph, they're only really useful for
helping desugaring, currently

[caitp (gmail), 2015-04-02 14:18:48]

Ports are written, nits addressed

https://codereview.chromium.org/938443002/diff/380002/src/harmony-spread.js
File src/harmony-spread.js (right):

https://codereview.chromium.org/938443002/diff/380002/src/harmony-spread.js#n...
src/harmony-spread.js:14: if (SpreadArguments) return;
On 2015/04/02 07:53:33, Dmitry Lomov (chromium) wrote:
> Why this 'if' is needed?

I wrote it this way to be able to share it with the spread-elements patch, but
I'll worry about that later

[arv (Not doing code reviews), 2015-04-02 15:19:33]

I'm also curious what your long term plan is? The desugaring is kind of heavy
and it would be better if we directly populated the stack instead of going
through temporary arrays.

[caitp (gmail), 2015-04-02 15:32:48]

On 2015/04/02 15:19:33, arv wrote:
> I'm also curious what your long term plan is? The desugaring is kind of heavy
> and it would be better if we directly populated the stack instead of going
> through temporary arrays.

It would be... That was actually the first approach taken, but it was a problem
for TF and not popular with the compiler guys. With dynamic argument counts, the
compiler is missing a lot of information, and might not be able to reason about
what to do if something goes wrong (among other things).

[titzer, 2015-04-07 08:18:04]

On 2015/04/02 15:32:48, caitp wrote:
> On 2015/04/02 15:19:33, arv wrote:
> > I'm also curious what your long term plan is? The desugaring is kind of
heavy
> > and it would be better if we directly populated the stack instead of going
> > through temporary arrays.
> 
> It would be... That was actually the first approach taken, but it was a
problem
> for TF and not popular with the compiler guys. With dynamic argument counts,
the
> compiler is missing a lot of information, and might not be able to reason
about
> what to do if something goes wrong (among other things).

Yes, the direct manipulation of dynamically-sized stack portions is an issue not
just for the compiler, but deoptimization, security, etc.

I think the long term solution should be to use a small but fixed size (e.g. 16)
stack-allocated buffer that can be used for small spreads and then overflow to
the full-blown array code in the uncommon case of large spread calls. Obviously,
if the compiler can figure out the static size of the spread, it should be able
to just expand it inline.

Thanks for taking the less efficient but more composable solution first; we can
do a better job later when more pieces fit together.

[Michael Starzinger, 2015-04-08 18:48:26]

LGTM. Didn't look at the parser and only glimpsed over the test coverage. Sorry
for the long delay, wasn't aware this CL was waiting on sign-off from my end.
Feel free to ping me when I don't respond.

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:198: void ALAA::VisitSpread(Spread*
e) { Visit(e->expression()); }
nit: Should be UNREACHABLE as well, correct?

[arv (Not doing code reviews), 2015-04-08 18:58:59]

LGTM

https://codereview.chromium.org/938443002/diff/450001/src/arm/full-codegen-ar...
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/938443002/diff/450001/src/arm/full-codegen-ar...
src/arm/full-codegen-arm.cc:4617: // Assert: expr ===
CallRuntime("ReflectConstruct")
DCHECK_EQ(expr->raw_name(), ...)

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:198: void ALAA::VisitSpread(Spread*
e) { Visit(e->expression()); }
On 2015/04/08 18:48:26, Michael Starzinger wrote:
> nit: Should be UNREACHABLE as well, correct?

I don't think so. The spread expression can be anything.

[caitp (gmail), 2015-04-08 19:02:54]

thanks for the review, will land this tonight

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
File src/compiler/ast-loop-assignment-analyzer.cc (right):

https://codereview.chromium.org/938443002/diff/450001/src/compiler/ast-loop-a...
src/compiler/ast-loop-assignment-analyzer.cc:198: void ALAA::VisitSpread(Spread*
e) { Visit(e->expression()); }
On 2015/04/08 18:58:59, arv wrote:
> On 2015/04/08 18:48:26, Michael Starzinger wrote:
> > nit: Should be UNREACHABLE as well, correct?
> 
> I don't think so. The spread expression can be anything.

Spread* nodes get completely removed from the graph, so it could just as well be
UNREACHABLE. I don't think it matters much though, since it will hit the other
ones if this line is reached

[caitp (gmail), 2015-04-09 14:59:05]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-04-09 14:59:05]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/938443002/#ps470001
(title: "rebase + clang-format")

[commit-bot: I haz the power, 2015-04-09 14:59:28]

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

[commit-bot: I haz the power, 2015-04-09 19:37:22]

Committed patchset #21 (id:470001)

[commit-bot: I haz the power, 2015-04-09 19:37:36]

Patchset 21 (id:??) landed as
https://crrev.com/74c381221c93253bbee513aa2539d93269303b8b
Cr-Commit-Position: refs/heads/master@{#27714}

[Michael Achenbach, 2015-04-10 10:04:19]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2015-04-10 10:04:20]

https://codereview.chromium.org/938443002/diff/470001/test/js-perf-test/JSTes...
File test/js-perf-test/JSTests.json (right):

https://codereview.chromium.org/938443002/diff/470001/test/js-perf-test/JSTes...
test/js-perf-test/JSTests.json:20: {"name": "Call-Sum"},
What is actually printed is (exemplified):
Call-SpreadCalls(Score): 123
CallMethod-SpreadCalls(Score): 123
CallNew-SpreadCalls(Score): 123

Therefore the perf runner fails to gather these results as it looks for e.g.
Call-Sum-SpreadCalls.

Repro:
make x64.release
tools/run_perf.py --arch x64 test/js-perf-test/JSTests.json