I actually think this is looking pretty good. Why do you think this is
fragile
(I mean, it at least doesn't seem any more fragile than the last two
iterations)?
My one big request is to de-generalize RewritableExpression (see comments in
ast.h and elsewhere).
https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h
File src/ast/ast.h (right):
https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h#newcode442
src/ast/ast.h:442: Expression* expr_;
Nit: just call this "expression_"
https://codereview.chromium.org/1309813007/diff/590001/src/ast/ast.h#newcode443
src/ast/ast.h:443: uint32_t bit_field_;
If you move this before expr_ and make it a uint8_t, it'll get packed in
with the 16 bit field in Expression (that seems to be the suggested
pattern elsewhere in ast.h)
Alternatively, this node is currently way more generic than necessary.
You could make it more specific to the current use case and simplify
much of the logic above, using a bool to track state instead of the
bitfield.
I think I prefer the latter; otherwise there are lots of places in the
code where we check for a RewriteHint that's always true. Probably
rename to RewritableAssignmentExpression in that case.
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc
File src/parsing/parser.cc (right):
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode4547
src/parsing/parser.cc:4547: RewritableExpression* rw =
expr->AsRewritableExpression();
Naming nit: I like "to_rewrite" better than "rw".
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode4548
src/parsing/parser.cc:4548: if (!rw ||
to_rewrite == nullptr
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode4549
src/parsing/parser.cc:4549:
!rw->IsRewritableAs(RewritableExpression::kDestructuringAssignment)) {
It's checks like this that I don't see as terribly useful, since they're
always true.
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode6525
src/parsing/parser.cc:6525: RewritableExpression* rw =
pair.assignment->AsRewritableExpression();
Like the old name better here too
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/parser.cc#newcode6539
src/parsing/parser.cc:6539:
DCHECK(expr->AsRewritableExpression()->expression()->IsAssignment());
This, too, will go away if you just use the C++ type Assignment in
RewritableExpression.
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/pattern-rewriter.cc
File src/parsing/pattern-rewriter.cc (right):
https://codereview.chromium.org/1309813007/diff/590001/src/parsing/pattern-rewriter.cc#newcode356
src/parsing/pattern-rewriter.cc:356: return Visit(node->expression());
I'd move this to the top and early return, letting you un-indent the
rest of this function.
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.
