On 2014/05/26 12:46:06, marja wrote:
Thanks for the update!
Here's a (non-exhaustive) list of comments.
I have been going over the items in the list and I have almost all of them
sorted
out now. (Some comments below, keep reading...)
[...]
- Test cases where you check that preparsing and parsing fail the same
way and
succeed for the same string (see test_parsing.cc)
After adding code for testing this, it turned out that the PreParser was not
behaving completely the same as the parser, in the case where the AST
subtree
from the arrow function arguments is passed to ParseArrowFunctionLiteral()
and
then examined to declare variables and report errors. The problem here was
that
the PreParser does not build an AST, so I came up with a solution which
works
by "bubbling up" flags in this way:
- Add flags to identify PreParserExpressions which are binary operations,
and to know when one of those encloses an identifier like "eval",
"arguments"
or any other future reserved identifier.
- PreParserExpression::BinaryOperation(left, right) is added, and it
returns
a PreParserExpression that makes an union of the flags in "left" and
"right".
Also, if a flag exists both in "left" and "right" (e.g. both enclose
"eval")
then it adds an additional flag to signal duplicates.
I have this working for "eval" and "arguments". As soon as I get it to work
also for future reserved keywords, both Parser and PreParser will be
generating
the same errors.
- 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?
No problem with this, it won't segfault anymore after fixing the way "()" is
handled. For example parsing "foo[()]" will stop and report that "=>" is the
next expected token, because in expressions an empty param list is only
valid
when followed by an arrow function body.
- Do constructs like (()) or (()) => something work as expected?
Yes, thanks to the ParamListFinder helper added to the scanner, which keeps
track if most recent parenthesized expression is a valid parameter list.
- Arrow functions in strict mode and the related errors (dupl. param names
etc.)
Read my comment above, about fixing the different errors reported in Parser
and PreParser. Parser was reporting those errors before, soon I will have
PreParser working as well.
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?
Correct, "]" is unexpected. Current version of the parsing code already
reports
this error properly.
I'm happy to accept a separate CL which splits the strict mode checking
out of
ParseFunctionLiteral where it currently is.
Sounds good to me. Let's try to have this one CL landed first, and then
refactor the checks, as you suggest.
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.
