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#newcode324
src/expression-classifier.h:324: if (b.location.beg_pos < 0 ||
b.location.beg_pos >= a.location.beg_pos) {
On 2015/10/28 19:11:41, adamk wrote:
Same here, I don't imagine pos < 0 is valid.
The above test doesn't guarantee that both errors are valid, only at
least one of them. So if one is invalid (eg beg_pos < 0), the other must
be valid.
They're tracked in separate errors because they're propagated
differently
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 {
On 2015/10/28 19:57:34, caitp wrote:
On 2015/10/28 19:11:41, adamk wrote:
> Looks like this snuck in from elsewhere? I don't see it used in this
patch.
Same
> with other marking infrastructure in this file.
Most of the changes in here aren't needed now, forgot about this one
Well, it's likely some variation of this will be needed for te actual
desugaring, but for now its gone
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#newcode2449
src/preparser.h:2449: if (peek() != Token::ARROW) {
On 2015/10/28 19:11:41, adamk wrote:
Why does this code have to be here instead of handling it like other
classifier
errors, in ParseMemberExpressionContinuation and
ParseAssignmentExpression?
`( expression ) [lookahead ∉ =>]` -> plain expression / possibly
DestructuringAssignment. There isn't enough context to figure this out
in ParseAssignmentExpression, because ParseExpression is not always part
of ParseAssignmentExpression.
in particular, this code used to use a version which always called
ValidateExpression(), but now it does this conditionally depending on
flags (which is a temporary measure). However, I suspect that it will
probably eagerly rewrite destructuring assignments if and only if any
are found here, so there will probably still be some extra code like
this in the future.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2532
src/preparser.h:2532: ExpressionT result = ParseExpression(accept_IN, 0,
classifier, CHECK_OK);
On 2015/10/28 19:11:41, adamk wrote:
Seems like 0 should have a name in this enum, too.
Acknowledged.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2622
src/preparser.h:2622: classifier->RecordPatternError(
On 2015/10/28 19:11:41, adamk wrote:
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.
It's an invalid binding pattern as well as invalid assignment pattern,
but there's no information about which one we think we're parsing, and
even if we think we're expecting an assignment, the `( expression )
[maybe =>]` case makes it ambiguous. So this one records the error for
both.
The only case, afaik, where it's an error for binding patterns but not
assignment patterns, is when the BindingElement is a property reference.
But adding a 3rd error class just for that seems like the wrong
approach.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2631
src/preparser.h:2631: if (argument->IsAssignment()) {
On 2015/10/28 19:11:41, adamk wrote:
Seems like this could be combined with the above check into a
ValidateDestructuringRestElement() helper function.
Isn't this going to be the only caller of that function, unless
https://github.com/sebmarkbage/ecmascript-rest-spread gets ratified? (I
guess it is at stage 2...)
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2638
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
On 2015/10/28 19:11:41, adamk wrote:
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?
Yeah, I think the message is better than "unexpected token...". I guess
the redundant code should be removed
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode3068
src/preparser.h:3068: bool maybe_binding_pattern = flags &
kMaybeBindingPattern;
On 2015/10/28 19:11:41, adamk wrote:
I think you don't need this at all, since I don't see it in a DCHECK
below.
It's used to determine whether destructuring assignment should be
eagerly rewritten. We can't eagerly rewrite or eagerly validate as an
AssignmentPattern if it could be a BindingPattern (as in left-most
ObjectLiteral / ArrayLiterals in parenthesized Expressions)
The USE() isn't really needed, though.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode3071
src/preparser.h:3071: flags & ~(kIsPatternElement |
kMaybeBindingPattern) | kIsRightHandSide;
On 2015/10/28 19:11:41, adamk wrote:
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.
Acknowledged.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode4339
src/preparser.h:4339: // TODO(caitp): do this in a less redundant way :(
On 2015/10/28 19:11:41, adamk wrote:
I take it this TODO is about the fact that this is redundant with
CheckAndRewriteReferenceExpression?
Yes, will clarify that
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode4340
src/preparser.h:4340: if (expression->IsValidReferenceExpression()) {
On 2015/10/28 19:11:41, adamk wrote:
I'd do an early return here to reduce indentation:
if (!expression->IsValidReferenceExpression) return false;
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.
