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;
On 2015/09/01 14:41:20, wingo wrote:
Is mutable needed? I think not, according to uses in this file.
It is needed by the pattern rewriter, because I was asked to remove the
AST node (although bmeurer seems to prefer the AST node --- I'm getting
confused by conflicting requests D:).
PatternRewriter touches mutable AST nodes, but because this (rest
parameters) isn't (currently) represented as an AST node, the Parser's
ReindexLiterals method needs to be able to update the literal index
appropriately.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode1067
src/preparser.h:1067: // Depends on
`ExpressionClassifier::is_valid_arrow_formal_parameters()`
On 2015/09/01 14:41:20, wingo wrote:
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."
Done.
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode1898
src/preparser.h:1898: }
On 2015/09/01 14:41:20, wingo wrote:
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.
I think we'd be better off getting rid of FormalParametersBase's
materialized_literal_count entirely, and solving this problem properly.
This will hopefully be cleaned up as a part of a fix for v8:4400, which
has been discussed on IRC a bit
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode2463
src/preparser.h:2463: ReportUnexpectedTokenAt(next_location, next);
On 2015/09/01 14:41:20, wingo wrote:
Why is this here? I can't reproduce this bug but I also don't see
where this
error case would be caught.
It produces a better error message --- it was moved as part of the AST
node patch set to produce the same message as the full parser, but can
be removed now.
Reverted this change
https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode2920
src/preparser.h:2920:
this->ParseArrowFunctionFormalParameterList(¶meters, expression,
loc,
On 2015/09/01 14:41:20, wingo wrote:
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?
It's not currently possible with the FormalParametersT + FunctionState
scheme. I agree that this would be better, but we need a better way to
track these in order to make that work.
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.
