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#newcode199
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#newcode55
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#newcode1021
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-function.cc
File src/runtime/runtime-function.cc (right):
https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-function.cc#newcode627
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/spread-call.js
File test/mjsunit/harmony/spread-call.js (right):
https://codereview.chromium.org/938443002/diff/20001/test/mjsunit/harmony/spread-call.js#newcode229
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
https://codereview.chromium.org/938443002/
--
--
v8-dev mailing list
[email protected]
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 emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
