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-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/938443002/diff/20001/src/compiler/ast-graph-builder.cc#newcode2231
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#newcode199
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#newcode9
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#newcode55
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#newcode95
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#newcode1021
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-function.cc
File src/runtime/runtime-function.cc (right):
https://codereview.chromium.org/938443002/diff/20001/src/runtime/runtime-function.cc#newcode624
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-function.cc#newcode627
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/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: })();
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
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.
