Some more nits. I defer to Adam's review of course.
https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4187
src/parser.cc:4187: }
No need for the if, just execute the body unconditionally, no?
Aside: it's pretty confusing that one field is
"materialized_literal_count" and the other is
"materialized_literals_count". Probably need to rename the latter to
the former, because the former has been around for longer.
https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4425
src/parser.cc:4425: auto var = parameters.scope->parameter(i);
On 2015/09/01 03:55:02, caitp wrote:
On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> Nit: Wow, this heavy autofication. I'm not sure what the convention
for the
> parser is, but I'd prefer to reduce the use of auto, esp. when it's
used in
> place of a pointer type.
My feeling is that it's alright for AST nodes (because of the
`New<NodeType>`
methods providing all the missing info), which is the bulk of the auto
use. If
folks want explicit typenames used everywhere, we can do that too.
How important is this, given that the majority of this is AST nodes?
I had this same thought when I read this FWIW; but it was only on this
one in particular (parameters.scope->parameter(i)). I don't know what
the type of var is; is it a proxy or a variable? I guess the latter but
since it's not a constructor a type would be appreciated.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode34
src/preparser.h:34: mutable int rest_array_literal_index = -1;
Is mutable needed? I think not, according to uses in this file.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode1067
src/preparser.h:1067: // Depends on
`ExpressionClassifier::is_valid_arrow_formal_parameters()`
I apologize for nitpicking but can we have sentences with full stops
please. I also find "depends on" to be a bit vague in this context. A
possibility:
"If (and only if) the expression classifier has determined that this
expression is a valid arrow formal parameter list, return true if the
formal parameter list has a rest parameter."
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode1898
src/preparser.h:1898: }
I am OK with this I guess. Is it possible, possibly in a followup, to
separate out a desurgaring phase which runs before parsing the function
body, in all cases? "Parse" should produce a FormalParametersT, and
ideally the desugaring could be in ParserBase. If you have to make an
array literal node for the rest array, the side effect on the
materialized literal count comes out naturally. Dunno, I recognize that
the parser is a gnarly thing onto which we keep hammering new things and
we can't always get it right, but fudging about with the materialized
literal count in four places smells wrong to me.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode2463
src/preparser.h:2463: ReportUnexpectedTokenAt(next_location, next);
Why is this here? I can't reproduce this bug but I also don't see where
this error case would be caught.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode2920
src/preparser.h:2920:
this->ParseArrowFunctionFormalParameterList(¶meters, expression,
loc,
Yeah moving this before the checkpoint just sounds like a bad idea to
me. I would prefer that the reservation of the literals index for the
array happened while parsing the function, within its scope and literals
count, and before parsing the function's body. Is that not possible?
https://codereview.chromium.org/1272673003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
