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) {
On 2015/08/28 23:26:37, adamk wrote:
Why isn't this one UNREACHABLE()?
I wasn't sure if AstExpressionVisitor was ever used before compilation
or not. Sounds like it isn't, so I guess it can probably be 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) {
On 2015/08/28 23:26:37, adamk wrote:
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.
Yeah, it's true that doing it all here could get rid of the new node ---
but it also means changing a lot of const references to non-const, and
maybe mutating stuff that you normally wouldn't expect to be mutated. It
could lead to bugs, but if the simplicity is worthwhile, I'm all for
that
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;
On 2015/08/28 23:26:37, adamk wrote:
Does this compile? I think this and the below should be initialized in
the
constructor.
Compiles with clang version 3.8.0 (trunk 242792), but will do. This
pattern is used in a few other structs in the tree though
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,
On 2015/08/28 23:26:37, adamk wrote:
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.
Sure, this way just seemed like less work than hunting down all the
callers. I'll make that change during the next round of cleanup
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode927
src/preparser.h:927: BinaryOperationTokenField::encode(op) |
On 2015/08/28 23:26:37, adamk wrote:
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.
Acknowledged
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode983
src/preparser.h:983: PreParserExpression AsVariableProxy() const {
return *this; }
On 2015/08/28 23:26:37, adamk wrote:
If you end up needing this, it seems like it should have a
DCHECK(IsIdentifier())
If it only returns false for `this`, that's probably okay. Acknowledged
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode1051
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) ==
Token::COMMA &&
On 2015/08/28 23:26:37, adamk wrote:
I'm not sure that you're getting much from this token field. Wouldn't
the
following expression also return true for HasRestParameter?
It's important for making lazy parsing work with arrow functions :( The
pre-parser needs to be able to figure out that it needs to increase the
materialized literal count for the new function. Without it, you end up
with a segfault, which isn't ideal :(
"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.
I think (hope) that `b + c` as a parameter would cause the classifier to
record an arrow formal parameters error. The flag is used only after the
parameters have been determined to be valid, so this situation doesn't
occur.
I could leave a comment indicating the conditions under which it can be
used, if that helps
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode2897
src/preparser.h:2897:
this->ParseArrowFunctionFormalParameterList(¶meters, expression,
loc,
On 2015/08/28 23:26:37, adamk wrote:
So this had to move up because it now affects
materialized_literals_count?
Yeah :( I tried to find a better way to do it, but storing this info in
the ExpressionClassifier was a bit problematic.
Would a comment help indicate the reasoning for moving it?
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode3866
src/preparser.h:3866:
DCHECK(formal_parameters.materialized_literals_count > 0);
On 2015/08/28 23:26:37, adamk wrote:
Would this ever be anything other than 1 in this case?
Yes --- I believe initializers with materialized literal values can
increase the value
In fact, it turns out that there's a bug in default parameters with lazy
parsing, so in fact we really need this to happen whenever
`materialized_literals_count > 0` and not just when has_rest is true.
I'll file a bug about that.
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.
