Just posted a new update. (BTW, if this CL is getting gets too big, I could
just cancel it and start a fresh one.)
On 2014/05/26 12:46:06, marja wrote:
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).
The tests are now into "cctest/test-parsing.cc". I have added some more
test cases with different contexts, but I will do one more update after
making sure arrow function syntax is enabled for all relevant test cases
and maybe adding some more test cases.
- Test cases where you check that preparsing and parsing fail the same
way and
succeed for the same string (see test_parsing.cc)
Those were now added.
- 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?
Those will not crash because in an expression the only thing that can
follow "()" is an arrow token, so the parser will report that "=>" was
expected but not found.
- Do constructs like (()) or (()) => something work as expected?
Yes. This is now handled properly. The relevant code is in the
"ParamListFinder" helper class (in scanner.cc): it is a small state
machine that tracks the most recent single identifier and the most
recent parenthesized parameter list, and is used to determine whether
the expression to the left side of "=>" is a valid parameter list.
- Arrow functions in strict mode and the related errors (dupl. param names
etc.)
All handled.
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?
That is correct, because after a "()" in an expression an arrow is expected.
In the middle of an expression, an empty "()" can only be followed by "=>".
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).
IMHO reporting that there is an unexpected token in those cases would be the
expected behavior. The tests now include some cases to check that "()" not
followed by "=>" results in a parsing error. WDYT?
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.
I have splitted the parts of the patch which are not related to parsing, to
post them in a separate CL. This new file will be in that new CL.
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.
