Excited to see this passing tests!
As you suspected, I'm a little bit skeptical of adding a new AST node for
RestParameter. I'm probably missing something, but it seems like a big
hammer
just to make reindexing work, as no other AST visitor will ever care about
it.
Not sure who else would be an appropriate reviewer, wingo@ is probably the
other
right person (besides rossberg, who'll be out for the next while). I'd say
dslomov, as he wrote the reindexer, but yeah :)
https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):
https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visitor.cc#newcode206
src/ast-expression-visitor.cc:206: void
AstExpressionVisitor::VisitRestParameter(RestParameter* expr) {
Why isn't this one UNREACHABLE()?
https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4012
src/parser.cc:4012: for (const auto p : parameters.params) {
So is const-correctness the only thing keeping you from handling rest
specially here, instead of adding a new AST node? The AST node adds a
good bit of cognitive overhead, and a very strange new type of
"Expression" (though I guess ExpressionClassifier already let that boat
sail).
If you could put the logic here, it seems like all you'd need is a field
in ParserFormalParameters::Parameter to store the literal index, and a
public method on AstLiteralReindexer to do the updating.
https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4416
src/parser.cc:4416: if (parameter.is_rest) {
Please add a comment here with the full desugaring, it's hard for me to
piece together among all the parser boilerplate.
https://codereview.chromium.org/1272673003/diff/60001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1272673003/diff/60001/src/parser.h#newcode1002
src/parser.h:1002: bool is_rest = false;
Does this compile? I think this and the below should be initialized in
the constructor.
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode708
src/preparser.h:708: inline void ParseFormalParameter(FormalParametersT*
parameters,
Is it really worth having two versions of this method? I don't think
there are so many callers that you couldn't just do the peek_position()
at the callsites and leave out the overload.
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode927
src/preparser.h:927: BinaryOperationTokenField::encode(op) |
This seems overly general. Can you get away with just:
HasRestField::encode(op == Token::COMMA && right->IsSpreadExpression())
It doesn't look to me like you're using the token for anything else; see
below on HasRestParameter for the rest of this comment.
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode983
src/preparser.h:983: PreParserExpression AsVariableProxy() const {
return *this; }
If you end up needing this, it seems like it should have a
DCHECK(IsIdentifier())
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode1051
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) ==
Token::COMMA &&
I'm not sure that you're getting much from this token field. Wouldn't
the following expression also return true for HasRestParameter?
"b + c, ...d"
Which clearly isn't a valid parameter list. Given the narrow usage of
this Token field, it seems like it might make more sense to directly
encode the bit you care about.
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode2897
src/preparser.h:2897:
this->ParseArrowFunctionFormalParameterList(¶meters, expression,
loc,
So this had to move up because it now affects
materialized_literals_count?
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode3866
src/preparser.h:3866:
DCHECK(formal_parameters.materialized_literals_count > 0);
Would this ever be anything other than 1 in this case?
https://codereview.chromium.org/1272673003/
--
--
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.
