I think this addresses all of the comments
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()?
All the AST node stuff is now gone, replaced with the use of the mutable
qualifier
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/29 01:01:07, adamk wrote:
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.
Done.
https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4416
src/parser.cc:4416: if (parameter.is_rest) {
On 2015/08/28 23:26:37, adamk wrote:
Please add a comment here with the full desugaring, it's hard for me
to piece
together among all the parser boilerplate.
Done.
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/29 01:01:07, adamk wrote:
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.
It turns out these members were unused anyways, so there isn't any
reason to keep them regardless. They're gone
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.
Method is gone, isn't needed without the AST node (might make errors
slightly uglier though)
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())
It's not needed without the new AST node, so it's gone anyways
https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode1051
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) ==
Token::COMMA &&
On 2015/08/29 01:01:07, adamk wrote:
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.
Done.
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/29 01:01:07, adamk wrote:
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.
I've changed it to always add the formals materialized literals whether
has_rest is true or not, but as noted in the bug I filed
(https://code.google.com/p/v8/issues/detail?id=4400) it doesn't actually
solve the problem for default arguments, which is a bit more
complicated.
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.
