Thanks for the update!
Here's a (non-exhaustive) list of comments.
In addition, pls add the following tests:
- Afaics there are no tests which make sure that PreParser handles arrow
functions properly! (mjsunit doesn't go via PreParser).
- Add the "arrow functions allowed" flag to test-parsing, so that all
existing
parsing tests are ran with arrow functions allowed (to make sure you're not
changing the accepted language).
- Test cases where you check that preparsing and parsing fail the same way
and
succeed for the same string (see test_parsing.cc)
- Test cases for "foo[some arrow func]" and another one for "foo[()]" (this
will
take the code path you added in ParseExpression, right?) Will these crash
because ParseExpression thinks () is okay and returns NULL?
- Do constructs like (()) or (()) => something work as expected?
- Arrow functions in strict mode and the related errors (dupl. param names
etc.)
- Tests I listed in the comments
- Lazy arrow functions
https://codereview.chromium.org/160073006/diff/480001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/160073006/diff/480001/src/parser.cc#newcode1017
src/parser.cc:1017: if (shared_info->is_arrow()) {
Probably the same refactoring applies to SharedInfo, instead of
"is_generator" and "is_arrow" you could have "type" (normal, generator,
arrow).
https://codereview.chromium.org/160073006/diff/480001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/160073006/diff/480001/src/parser.h#newcode388
src/parser.h:388: typedef v8::internal::Scope* ScopePtr;
Why is this needed?
https://codereview.chromium.org/160073006/diff/480001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/160073006/diff/480001/src/preparser.h#newcode1607
src/preparser.h:1607: CHECK_OK);
I think there is a bug here.
So if we're here... but it's not an arrow function but a stray "()",
what will happen? I think you're changing the existing behavior of ()
here. And that's not intentional, I guess?
For example, if the code is foo[()], we currently produce an error
"Unexpected token )", but now, if arrow functions are allowed, we'll
produce an error "Unexpected token ]", right?
In any case, pls add test code which tests that possibility (and even
better, a test which would've exposed the bug, if you agree it's a
bug... but that might be difficult).
https://codereview.chromium.org/160073006/diff/480001/src/preparser.h#newcode1643
src/preparser.h:1643: if (allow_arrow_functions() && peek() ==
Token::RPAREN &&
Pls add a test where this code path is taken but it turns out to not be
an arrow function.
https://codereview.chromium.org/160073006/diff/480001/src/preparser.h#newcode2314
src/preparser.h:2314: ParserBase<Traits>::ParseArrowFunctionLiteralBody(
A lot of this function (starting from strict mode checking) is duplicate
code; pls refactor.
I'm happy to accept a separate CL which splits the strict mode checking
out of ParseFunctionLiteral where it currently is.
https://codereview.chromium.org/160073006/diff/480001/test/mjsunit/harmony/arrow-functions-parsing.js
File test/mjsunit/harmony/arrow-functions-parsing.js (right):
https://codereview.chromium.org/160073006/diff/480001/test/mjsunit/harmony/arrow-functions-parsing.js#newcode28
test/mjsunit/harmony/arrow-functions-parsing.js:28: var
objectLiteralResult = ({ value: 42 });
Pls test that these yield the expected results, too.
https://codereview.chromium.org/160073006/diff/480001/test/mjsunit/harmony/arrow-functions-parsing.js#newcode39
test/mjsunit/harmony/arrow-functions-parsing.js:39: assertThrows(");",
SyntaxError);
You can move these "parsing should fail" cases to test-parsing.cc.
https://codereview.chromium.org/160073006/diff/480001/test/mjsunit/harmony/arrow-functions-parsing.js#newcode59
test/mjsunit/harmony/arrow-functions-parsing.js:59: assertThrows("(x,
(y)) => 0;", SyntaxError);
You should add tests for different (legal and illegal) contexts where
arrow functions (or something resembling them) can occur.
For example,
foo, bar => baz
foo ? bar => ... : ...
foo[()]
foo[bar => baz]
In addition, you should test cases like
((foo, bar, baz)) => ...
(foo, (bar, baz)) => ...
((foo, bar), baz) => ...
https://codereview.chromium.org/160073006/
--
--
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.
