I'll get to these in a bit, adding some more tests first
https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):
https://codereview.chromium.org/1309813007/diff/470001/src/ast-expression-visitor.cc#newcode270
src/ast-expression-visitor.cc:270:
RECURSE_EXPRESSION_RETURN_IF_VISIT_NODE(expr->destructuring_assignment());
On 2015/11/25 21:05:28, adamk wrote:
Is this needed because VisitExpression could mutate |expr|?
I suppose it can, the InitializerRewriter visitor does that, I think.
AFAIK the top-most RETURN_IF_VISIT_NODE probably should be removed, but
I'm not positive
https://codereview.chromium.org/1309813007/diff/470001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2379
src/ast.h:2379: Expression* destructuring_assignment() { return
destructuring_assignment_; }
On 2015/11/25 21:05:28, adamk wrote:
Looks like this is a non-const dup of the above accessor, I don't
think you need
it.
Acknowledged.
https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2402
src/ast.h:2402: : public BitField16<bool, TokenField::kNext, 1> {};
On 2015/11/25 21:05:28, adamk wrote:
Can you use kNext for the above lines while you're at it?
Acknowledged.
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1095
src/parser.h:1095: PatternContext
SetInitializerContextIfNeeded(Expression* node) {
On 2015/11/25 21:05:28, adamk wrote:
Any reason all of these functions need to be in the .h file? If it's
for
inlining, that's not needed; these are private methods and all their
callers are
on this class, so they could just as well be inline and defined in the
.cc file.
No real reason, it was just convenient while writing them. I'll move
them
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1496
src/parser.h:1496: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
On 2015/11/25 21:05:28, adamk wrote:
And here...
Also, as mentioned before, this name is confusing. How about
"QueueDestructuringAssignmentForRewriting" (also long, but clearer
what it's
doing).
Acknowledged.
https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):
https://codereview.chromium.org/1309813007/diff/470001/src/pattern-rewriter.cc#newcode276
src/pattern-rewriter.cc:276: set_context(context);
On 2015/11/25 21:05:28, adamk wrote:
Can you use an RAII object for this instead of relying on set_context
at the end
of the block to handle this?
Doable, but it is (relatively) a lot of code and I don't think it needs
to be used a whole lot. I'll put that in the next patch
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode279
src/preparser.h:279: List<DestructuringAssignment>
destructuring_assignments_to_rewrite_;
On 2015/11/25 21:05:28, adamk wrote:
Please move this to Parser unless there's some reason it's needed in
the
preparser.
The list lives in FunctionState, so each FunctionState has its own
collection of assignments to rewrite, and does once the function is
ready to be made into an AST node.
The List is always empty in the PreParser (eg, manipulating the list is
Parser-specific). It might be possible to declare it as like, a void
type or something in the PreParser, but I'm not positive that would
realy work. Thoughts?
I originally stored the list in the Parser and tried to do the rewriting
once ParseProgram had finished (if it had been called), but had issues
with that.
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode281
src/preparser.h:281: void RewriteDestructuringAssignments();
On 2015/11/25 21:05:28, adamk wrote:
I don't see this defined anywhere (and I don't think it needs to be
defined).
Acknowledged.
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode2493
src/preparser.h:2493: if (peek() != Token::ARROW) {
On 2015/11/25 21:05:28, adamk wrote:
Sorry, I've forgotten if you've answered this already: why are doing
this
peeking here now (what does it have to do with parsing destructuring
assignment,
that is)?
```
// Valid if: --harmony-destructuring-assignment
var o = ({ a: b } = someObj);
// Valid if: --harmony-default-parameters +
--harmony-destructuring-bind, only if followed by `=>`
var f = ({ a: b } = someObj) => b;
```
Can't eagerly break the initializer in ParseAssignmentExpression unless
we know if the ARROW is there or not, so it defers to the caller and
does it here
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode2632
src/preparser.h:2632: bool seen_spread = false;
On 2015/11/25 21:05:28, adamk wrote:
This bool is now unused.
Acknowledged.
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode3108
src/preparser.h:3108: USE(is_arrow_formals);
On 2015/11/25 21:05:28, adamk wrote:
This USE is not needed.
Acknowledged.
https://codereview.chromium.org/1309813007/
--
--
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.
