The default bug view has changed. See this FAQ.

Factor out TokenMatcher::matchContextualKeyword

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 765276 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword
(Assignee)

Updated

4 years ago
Assignee: general → wingo
(Assignee)

Comment 2

4 years ago
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.
Attachment #765276 - Flags: review?(jwalden+bmo)
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.
Attachment #765276 - Flags: review?(jwalden+bmo) → review+
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.
(Assignee)

Comment 5

4 years ago
Created attachment 765818 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword (v2)
(Assignee)

Updated

4 years ago
Attachment #765276 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 765835 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
(Assignee)

Updated

4 years ago
Attachment #765818 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Thanks for the review.  I reflowed the conditional, gave the patch a commit message, and set the checkin-needed keyword.
Keywords: checkin-needed
Patch does not apply to inbound cleanly.
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
Created attachment 766578 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
(Assignee)

Updated

4 years ago
Attachment #765835 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Rebased patch for conflicts with bug 883333; should apply cleanly now.
Keywords: checkin-needed
(Assignee)

Comment 11

4 years ago
Created attachment 766579 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
(Assignee)

Updated

4 years ago
Attachment #766578 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Sorry for the noise; the previous patch wasn't at the bottom of my hq queue.  The latest attachment is the one.
This doesn't apply cleanly to mozilla-inbound. Please rebase.
Keywords: checkin-needed
(Assignee)

Comment 14

4 years ago
Created attachment 766731 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
(Assignee)

Updated

4 years ago
Attachment #766579 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Trying checkin-needed again after pulling inbound.
Keywords: checkin-needed
I think you forgot to qref before attaching. This is identical to the last one you attached and still conflicts.
Keywords: checkin-needed
(Assignee)

Comment 17

4 years ago
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?
(Assignee)

Comment 18

4 years ago
Created attachment 767672 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
(Assignee)

Updated

4 years ago
Attachment #766731 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
OK!  After some kind help by Ms2ger on IRC I finally managed to rebase correctly with hg.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7394903dd3

Thanks for sticking with it, Andy!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca7394903dd3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 879079
You need to log in before you can comment on or make changes to this bug.