I understand more and more each time, quite exciting.
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());
Is this needed because VisitExpression could mutate |expr|?
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_; }
Looks like this is a non-const dup of the above accessor, I don't think
you need it.
https://codereview.chromium.org/1309813007/diff/470001/src/ast.h#newcode2402
src/ast.h:2402: : public BitField16<bool, TokenField::kNext, 1> {};
Can you use kNext for the above lines while you're at it?
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) {
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.
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1473
src/parser.h:1473: void Parser::RewriteDestructuringAssignments() {
Another "why is this in the header?" question.
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1496
src/parser.h:1496: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
And here...
Also, as mentioned before, this name is confusing. How about
"QueueDestructuringAssignmentForRewriting" (also long, but clearer what
it's doing).
https://codereview.chromium.org/1309813007/diff/470001/src/parser.h#newcode1506
src/parser.h:1506: inline void
Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* node) {
And here and the below. parser.h is long enough as it is :)
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);
Can you use an RAII object for this instead of relying on set_context at
the end of the block to handle this?
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_;
Please move this to Parser unless there's some reason it's needed in the
preparser.
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode281
src/preparser.h:281: void RewriteDestructuringAssignments();
I don't see this defined anywhere (and I don't think it needs to be
defined).
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode2493
src/preparser.h:2493: if (peek() != Token::ARROW) {
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)?
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode2632
src/preparser.h:2632: bool seen_spread = false;
This bool is now unused.
https://codereview.chromium.org/1309813007/diff/470001/src/preparser.h#newcode3108
src/preparser.h:3108: USE(is_arrow_formals);
This USE is not needed.
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.
