Another source of tests for this would be the test262 tests in
language/expressions/assignment/destructuring (currently all skipped).
Here are some comments on the current patch. This looks like a good
direction to
me; I'd really like to see stuff moved up into Assignment, though (I've
noted
another problem with the current approach: the strange treatment of
AssignmentPattern when it's visited by various AST visitors).
https://codereview.chromium.org/1309813007/diff/350001/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):
https://codereview.chromium.org/1309813007/diff/350001/src/ast-expression-visitor.cc#newcode235
src/ast-expression-visitor.cc:235:
RECURSE_EXPRESSION(Visit(expr->expression()));
For each visitor, how did you decide whether to visit the expression vs
the pattern? This is another reason why it would make more sense to
handle this in VisitAssignment, it's sort of meaningless to visit an
assignment pattern without more context.
https://codereview.chromium.org/1309813007/diff/350001/src/crankshaft/hydrogen.cc
File src/crankshaft/hydrogen.cc (right):
https://codereview.chromium.org/1309813007/diff/350001/src/crankshaft/hydrogen.cc#newcode5864
src/crankshaft/hydrogen.cc:5864: void
HOptimizedGraphBuilder::VisitAssignmentPattern(AssignmentPattern* expr)
{
I don't think this is sufficient to bail out of Crankshaft. I suspect
you need to modify VisitAssignment, otherwise you'll get DCHECK failures
there.
https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/1309813007/diff/350001/src/flag-definitions.h#newcode678
src/flag-definitions.h:678: DEFINE_BOOL(parallel_compaction, true, "use
parallel compaction")
Looks like a stray change here
https://codereview.chromium.org/1309813007/diff/350001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1430
src/parser.h:1430: for (int i = assignments.length() - 1; i >= 0; --i) {
Any particular reason for iterating backwards here?
https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1451
src/parser.h:1451: void
ParserTraits::ShouldRewriteDestructuringAssignment(Expression* expr) {
The name of this function doesn't make sense to me. Perhaps
"QueueDestructuringAssignmentForRewriting"? "Should" prefix suggests a
const method that answers a question.
https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1452
src/parser.h:1452: DCHECK(expr->IsAssignment());
Why can't this method just take an Assignment* to avoid this DCHECK?
https://codereview.chromium.org/1309813007/diff/350001/src/parser.h#newcode1460
src/parser.h:1460: inline void
Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* node) {
Can these methods move to the .cc file? Seems like this header's already
plenty big.
https://codereview.chromium.org/1309813007/diff/350001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):
https://codereview.chromium.org/1309813007/diff/350001/src/pattern-rewriter.cc#newcode432
src/pattern-rewriter.cc:432: DoExpression* expr =
factory()->NewDoExpression(block_, temp, pos);
Looks like this creates a DoExpression even for destructured binding?
Seems weird to do that only to wrap it in an ExpressionStatement
below...
https://codereview.chromium.org/1309813007/diff/350001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1309813007/diff/350001/src/preparser.h#newcode279
src/preparser.h:279: List<DestructuringAssignment>
destructuring_assignments_to_rewrite_;
Why does ParserBase need this List? Couldn't it just be in Parser?
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.
