Great start.
https://codereview.chromium.org/816913003/diff/80001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/816913003/diff/80001/src/ast.h#newcode405
src/ast.h:405: void set_ellipsis(int location) {
This is a bit strange. Normally we would introduce a new ast node for
this.
https://codereview.chromium.org/816913003/diff/80001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/816913003/diff/80001/src/compiler.cc#newcode608
src/compiler.cc:608: int parameter_count = lit->parameter_count();
Maybe add something to FunctionLiteral that returns the static function
length based on rest (now) and default (future) params.
https://codereview.chromium.org/816913003/diff/80001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/816913003/diff/80001/src/parser.cc#newcode3508
src/parser.cc:3508: bool CheckAndDeclareArrowParameter(ParserTraits*
traits, Expression* expression,
I would be totally fine with not supporting rest in arrow functions at
this point... but it is good that we keep these in mind.
https://codereview.chromium.org/816913003/diff/80001/src/parser.cc#newcode3539
src/parser.cc:3539: bool is_rest_param = *is_rest =
expression->has_ellipsis();
If/when we add real SpreadExpression AST nodes...
`expression->IsSpread()` maybe?
https://codereview.chromium.org/816913003/diff/80001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/816913003/diff/80001/src/preparser.h#newcode2780
src/preparser.h:2780: // arw malformed.
typo
https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):
https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scopes.cc#newcode504
src/runtime/runtime-scopes.cc:504: Object** parameters,
Why not Arguments here?
https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scopes.cc#newcode512
src/runtime/runtime-scopes.cc:512: elements->set(i, *--parameters);
I think this would be cleaner as *args[i + rest_index] or something like
that.
https://codereview.chromium.org/816913003/diff/80001/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/816913003/diff/80001/src/scanner.cc#newcode602
src/scanner.cc:602: if (c0_ == '.') {
Do we want to do the flag check here?
https://codereview.chromium.org/816913003/diff/80001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/816913003/diff/80001/src/scopes.h#newcode373
src/scopes.h:373: return nullptr;
Maybe use DCHECK instead of returning nullptr?
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/rest-params.js
File test/mjsunit/harmony/rest-params.js (right):
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/rest-params.js#newcode7
test/mjsunit/harmony/rest-params.js:7: function strict_test(a, b, ...c)
{
strict_test -> strictTest
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/rest-params.js#newcode10
test/mjsunit/harmony/rest-params.js:10: assertTrue(Array.isArray(c));
maybe check Object.getPrototypeOf too?
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/rest-params.js#newcode12
test/mjsunit/harmony/rest-params.js:12: var expected_length =
arguments.length >= 3 ? arguments.length - 2 : 0;
no underscores
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/rest-params.js#newcode16
test/mjsunit/harmony/rest-params.js:16: //assertEquals(c[j++],
arguments[i]);
?
Is this not working yet?
https://codereview.chromium.org/816913003/
--
--
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.
