Understanding more and more (or so I think)...
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h
File src/expression-classifier.h (right):
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode319
src/expression-classifier.h:319: static const Error& FirdRecorded(const
Error& a, const Error& b) {
Did you mean "FirstRecorded"? Given that there's only a single caller,
not sure it's worth having a whole method here, could just be in
ValidateExpression.
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode323
src/expression-classifier.h:323: if (a.location.beg_pos < 0) return b;
Isn't this covered by the DCHECK above?
https://codereview.chromium.org/1309813007/diff/210001/src/expression-classifier.h#newcode324
src/expression-classifier.h:324: if (b.location.beg_pos < 0 ||
b.location.beg_pos >= a.location.beg_pos) {
Same here, I don't imagine pos < 0 is valid.
https://codereview.chromium.org/1309813007/diff/210001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1309813007/diff/210001/src/parser.cc#newcode3363
src/parser.cc:3363: return expression;
Probably keep the TODO here you had before.
https://codereview.chromium.org/1309813007/diff/210001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1309813007/diff/210001/src/parser.h#newcode563
src/parser.h:563: struct MarkedAssignment {
Looks like this snuck in from elsewhere? I don't see it used in this
patch. Same with other marking infrastructure in this file.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode711
src/preparser.h:711: enum AssignmentFlags {
This enum deserves a comment, along with a comment for each value. In
particular I find the usage of kIsPatternElement somewhat
counter-intuitive.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2449
src/preparser.h:2449: if (peek() != Token::ARROW) {
Why does this code have to be here instead of handling it like other
classifier errors, in ParseMemberExpressionContinuation and
ParseAssignmentExpression?
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2532
src/preparser.h:2532: ExpressionT result = ParseExpression(accept_IN, 0,
classifier, CHECK_OK);
Seems like 0 should have a name in this enum, too.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2612
src/preparser.h:2612: if (!allow_harmony_spread_arrays() &&
!allow_harmony_destructuring() &&
FYI spread arrays flag is gone so this if statement is too.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2622
src/preparser.h:2622: classifier->RecordPatternError(
Why is this not directly calling RecordAssignmentPatternError? Were we
getting this wrong before, or does this have to do with the ambiguity in
arrow params?
Same question for the below calls to RecordPatternError.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2631
src/preparser.h:2631: if (argument->IsAssignment()) {
Seems like this could be combined with the above check into a
ValidateDestructuringRestElement() helper function.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2638
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
This error is already handled for binding patterns down below, see "if
(seen_spread)". Is the idea that you're giving a better error message
here?
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode3068
src/preparser.h:3068: bool maybe_binding_pattern = flags &
kMaybeBindingPattern;
I think you don't need this at all, since I don't see it in a DCHECK
below.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode3071
src/preparser.h:3071: flags & ~(kIsPatternElement |
kMaybeBindingPattern) | kIsRightHandSide;
Please split across two lines (&= and then |=), my brain can do bitwise
ops much more easily that way.
I think a comment would also be worthwhile above those two lines to say
what you're doing.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode4339
src/preparser.h:4339: // TODO(caitp): do this in a less redundant way :(
I take it this TODO is about the fact that this is redundant with
CheckAndRewriteReferenceExpression?
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode4340
src/preparser.h:4340: if (expression->IsValidReferenceExpression()) {
I'd do an early return here to reduce indentation:
if (!expression->IsValidReferenceExpression) return false;
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.
