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#newcode2622
src/preparser.h:2622: classifier->RecordPatternError(
On 2015/10/29 23:43:21, adamk wrote:
On 2015/10/29 16:38:41, caitp wrote:
> 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.
Sorry, I didn't highlight the most important question: was this
already broken
for binding patterns? What confuses me about these checks is that I
would have
assumed that our destructuring binding tests would already exercise
these paths.
It's old code now, presumably something failed at some point, maybe
still does if it's removed. OTOH I don't know, though.
https://codereview.chromium.org/1309813007/diff/210001/src/preparser.h#newcode2638
src/preparser.h:2638: MessageTemplate::kElementAfterRest);
On 2015/10/29 23:43:21, adamk wrote:
On 2015/10/29 16:38:41, caitp wrote:
> 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
There are plenty of existing places where we use
BindingPatternUnexpectedToken,
it's not clear to me why this case should be special.
For one thing, the diagnostic is inaccurate. It's not an "unexpected
token", the parser is explicitly allowing this token to occur, and
reporting an error depending on context. So, given that, why not make
the error actionable, by relating it to the context that it depends on?
This is what makes diagnostic messages useful.
This doesn't apply to every use of BindingPatternUnexpectedToken(), but
it does apply to a few of them
https://codereview.chromium.org/1309813007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
