885281 - Factor out TokenMatcher::matchContextualKeyword

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword

Comment 2

[Andy Wingo [:wingo]]

Comment on attachment 765276 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword

Attached patch factors out matchContextualKeyword, and uses it in for-of.  It will also be used in matching "yield" inside "function*" in non-JS1.8 mode.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 765276 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword

Review of attachment 765276 [details] [diff] [review]:
-----------------------------------------------------------------

Good eye.

::: js/src/frontend/Parser.cpp
@@ +3650,5 @@
>      pn->pn_iflags = 0;
>  
> +    if (allowsForEachIn() &&
> +        tokenStream.matchContextualKeyword(context->names().each))
> +        pn->pn_iflags = JSITER_FOREACH;

The whole condition can fit on one 99ch line, so do that.  (If it couldn't, you'd have to add braces [aligned with the 'i' in 'if'] to the body, to more clearly delineate the body.)

@@ +5910,5 @@
>          pn2->setOp(JSOP_ITER);
>          pn2->pn_iflags = JSITER_ENUMERATE;
> +        if (allowsForEachIn() &&
> +            tokenStream.matchContextualKeyword(context->names().each))
> +            pn2->pn_iflags |= JSITER_FOREACH;

Same.

Comment 4

[Jeff Walden [:Waldo]]

jorendorff has a stack'o'patches in bug 883333 that slightly conflict with this in the for-statement parsing method parts.  You can rebase a lot more easily than he can, so I'd suggest waiting til he's landed his stack.

Comment 5

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword (v2)

Comment 6

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword.

Comment 7

[Andy Wingo [:wingo]]

Thanks for the review.  I reflowed the conditional, gave the patch a commit message, and set the checkin-needed keyword.

Comment 8

[(no longer active)]

Patch does not apply to inbound cleanly.

Comment 9

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword.

Comment 10

[Andy Wingo [:wingo]]

Rebased patch for conflicts with bug 883333; should apply cleanly now.

Comment 11

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword.

Comment 12

[Andy Wingo [:wingo]]

Sorry for the noise; the previous patch wasn't at the bottom of my hq queue.  The latest attachment is the one.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to mozilla-inbound. Please rebase.

Comment 14

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword.

Comment 15

[Andy Wingo [:wingo]]

Trying checkin-needed again after pulling inbound.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I think you forgot to qref before attaching. This is identical to the last one you attached and still conflicts.

Comment 17

[Andy Wingo [:wingo]]

How frustrating.  I'm sorry I'm wasting your time here; I have popped all patches, pushed just that one, and hg diff shows nothing.  I did "hg bzexport" to attach the patch.  Is this the wrong thing?

Comment 18

[Andy Wingo [:wingo]]

Attachment: Factor out TokenMatcher::matchContextualKeyword.

Comment 19

[Andy Wingo [:wingo]]

OK!  After some kind help by Ms2ger on IRC I finally managed to rebase correctly with hg.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7394903dd3

Thanks for sticking with it, Andy!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca7394903dd3