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:58:01, caitp wrote:
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
The current thing is cheating quite a bit, const-wise: it only works
because the parameters hold non-const pointers to Expression*. I'm not
sure why FormalParametersT is passed by const-ref everywhere while
ExpressionClassifier, for example, is passed by mutable-pointer. To get
a minimal change going you could even use the 'mutable' keyword on the
bit you need to change in a Parameter.
So, fwiw, my taste would be to avoid a new AST node here.
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:58:01, caitp wrote:
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
Huh, I may just be out of date. Feel free to leave this as-is if we're
already doing this elsewhere.
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#newcode1051
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) ==
Token::COMMA &&
On 2015/08/28 23:58:01, caitp wrote:
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
I think I wasn't as clear as I could have been. I understand what you're
trying to detect here (and why it's necessary), I just don't think you
need to encode as much information as you're currently encoding.
Basically, the only binary op whose token you care about is the one
whose RHS is a Spread. So you should be able to handle the COMMA check
when you're creating that binary op.
The advantage of doing it that way is you don't need 8 bits of
PreParserExpression space.
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:58:01, caitp wrote:
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?
No need for a comment, as long as a test will fail if these are
reordered again. I was mostly checking that my assumption was correct;
after this patch, this order seems fine too.
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:58:01, caitp wrote:
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.
Ah, I meant to say "can this ever happen without default params", but I
guess we're pretty much living in that world already. Not surprised to
hear there are more bugs in that stuff, look forward to your report.
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.
