https://codereview.chromium.org/160073006/diff/380001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3139
src/parser.cc:3139: Vector<VariableProxy*>
Parser::ParameterListFromExpression(
On 2014/03/25 12:35:34, rossberg wrote:
How does this rule out illegal parenthesis? For example, ((x)) or ((x,
y)) would
seem to (incorrectly) parse as the parameter lists (x) and (x, y),
wouldn't it?
True, this case is tricky to implement when re-interpreting an
already-parsed
AST as a parameter list. Probably this is going to need some way of
tracking
whether a parenthesized expression is inside another. I am going to add
a
TODO here for the moment.
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3141
src/parser.cc:3141: if (expression == NULL)
On 2014/03/24 09:04:06, marja wrote:
Normally having expression == NULL would be an error case; pls add a
comment
here that this can actually happen in non-error cases. (Afaics it
happens when
the argument list is empty.)
Yes, when the argument list is empty, "expression" is NULL. I will add
a comment about NULL being valid in this particular case.
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3151
src/parser.cc:3151: ParserTraits::ReportMessageAt(Scanner::Location(pos,
pos),
On 2014/03/25 12:35:34, rossberg wrote:
Can we refactor this a little to avoid all the error reporting code
duplication?
Sure.
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3312
src/parser.cc:3312: // FormalParameterList ::
On 2014/03/25 12:35:34, rossberg wrote:
On 2014/03/24 09:04:06, marja wrote:
> This ParseFunctionLiteral is pretty monstrous as is, can it be split
first
> before adding another dimension for array funcs?
+1. The arrow function case is parsing quite different syntax, so I'd
rather
have it as a separate function. You can still factor out commonalities
into
third functions (e.g. ParseFunctionParameterList, ParseFunctionBody,
etc.).
I am in the process of moving parsing of arrow functions to a different
function, and trying to put it in ParserBase, which is making me having
to move/add some bits into the common parser stuff -- IMHO this better
for
new code than having to convert things later on.
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3315
src/parser.cc:3315: if (func_parsing_mode == kArrowFunction && peek() ==
Token::ARROW) {
On 2014/03/25 12:35:34, rossberg wrote:
Can it ever happen that the first condition is true but the second
isn't? Where
do you invoke ParseFunctionLiteral with kArrowFunction but have not
parsed the
parameters yet?
It could happen when the function is parsed lazily: it is known in
advance
that we would be parsing an arrow function
(SharedFunctionInfo->is_arrow()
returns true), and we would start parsing at the start of the parameter
list
(so peek() != Token::ARROW). Anyway, after moving arrow function parsing
to
its own function this code will not be here anymore.
https://codereview.chromium.org/160073006/diff/380001/src/parser.cc#newcode3400
src/parser.cc:3400: Expression* expression = ParseExpression(true,
CHECK_OK);
On 2014/03/25 12:35:34, rossberg wrote:
Shouldn't this be ParseAssignmentExpression?
Indeed.
https://codereview.chromium.org/160073006/diff/380001/src/preparser.cc
File src/preparser.cc (right):
https://codereview.chromium.org/160073006/diff/380001/src/preparser.cc#newcode844
src/preparser.cc:844: PreParser::Expression
PreParser::ParseFunctionLiteral(
On 2014/03/24 09:04:06, marja wrote:
... this doesn't handle arrow funcs yet? (Like, single statement body
and so
on.)
You should add tests to cctest/test-parsing to check that the parser
and
preparser accept the same set of arrow functions.
FWIW, I will add the tests, but provided that I am will be adding a new
ParseArrowFunctionLiteral() in ParserBase, both parser and preparser
will
be in sync.
https://codereview.chromium.org/160073006/diff/380001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/160073006/diff/380001/src/preparser.h#newcode1464
src/preparser.h:1464: if (allow_arrow_functions() && peek() ==
Token::RPAREN) {
On 2014/03/24 09:04:06, marja wrote:
This doesn't sound right. I guess the idea is that we see the (, then
parse the
expression inside the () with ParseExpression. But what if we see a
lonely )
when we expect an expression? That should be an error, but this looks
like it'll
accept it.
It was causing an unexpected token error most of the time, because of
the requirement that parens are balanced, but there was a case (parsing
a single expression-statement) which caused infinite recursion. I am
updating this to check for:
scanner()->current_token() == Token::LPAREN && peek() == Token::RPAREN
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.
