Last Comment Bug 885281 - Factor out TokenMatcher::matchContextualKeyword
: Factor out TokenMatcher::matchContextualKeyword
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Andy Wingo [:wingo]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 879079
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-20 03:30 PDT by Andy Wingo [:wingo]
Modified: 2013-06-26 14:38 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Factor out TokenMatcher::matchContextualKeyword (3.82 KB, patch)
2013-06-20 03:31 PDT, Andy Wingo [:wingo]
jwalden+bmo: review+
Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword (v2) (3.79 KB, patch)
2013-06-21 01:14 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword. (3.83 KB, patch)
2013-06-21 01:37 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword. (3.99 KB, patch)
2013-06-24 01:41 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword. (3.99 KB, patch)
2013-06-24 01:44 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword. (3.99 KB, patch)
2013-06-24 09:31 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review
Factor out TokenMatcher::matchContextualKeyword. (3.95 KB, patch)
2013-06-26 02:54 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review

Description Andy Wingo [:wingo] 2013-06-20 03:30:25 PDT

    
Comment 1 Andy Wingo [:wingo] 2013-06-20 03:31:01 PDT
Created attachment 765276 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword
Comment 2 Andy Wingo [:wingo] 2013-06-20 03:33:37 PDT
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] (remove +bmo to email) 2013-06-20 19:30:38 PDT
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] (remove +bmo to email) 2013-06-20 19:31:52 PDT
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] 2013-06-21 01:14:50 PDT
Created attachment 765818 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword (v2)
Comment 6 Andy Wingo [:wingo] 2013-06-21 01:37:46 PDT
Created attachment 765835 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
Comment 7 Andy Wingo [:wingo] 2013-06-21 01:43:57 PDT
Thanks for the review.  I reflowed the conditional, gave the patch a commit message, and set the checkin-needed keyword.
Comment 8 :Ehsan Akhgari 2013-06-22 05:51:21 PDT
Patch does not apply to inbound cleanly.
Comment 9 Andy Wingo [:wingo] 2013-06-24 01:41:13 PDT
Created attachment 766578 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
Comment 10 Andy Wingo [:wingo] 2013-06-24 01:43:33 PDT
Rebased patch for conflicts with bug 883333; should apply cleanly now.
Comment 11 Andy Wingo [:wingo] 2013-06-24 01:44:23 PDT
Created attachment 766579 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
Comment 12 Andy Wingo [:wingo] 2013-06-24 01:46:38 PDT
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] 2013-06-24 08:31:08 PDT
This doesn't apply cleanly to mozilla-inbound. Please rebase.
Comment 14 Andy Wingo [:wingo] 2013-06-24 09:31:30 PDT
Created attachment 766731 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
Comment 15 Andy Wingo [:wingo] 2013-06-24 09:33:52 PDT
Trying checkin-needed again after pulling inbound.
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-06-25 14:57:56 PDT
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] 2013-06-26 02:24:22 PDT
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] 2013-06-26 02:54:11 PDT
Created attachment 767672 [details] [diff] [review]
Factor out TokenMatcher::matchContextualKeyword.
Comment 19 Andy Wingo [:wingo] 2013-06-26 02:56:02 PDT
OK!  After some kind help by Ms2ger on IRC I finally managed to rebase correctly with hg.
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-06-26 06:47:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7394903dd3

Thanks for sticking with it, Andy!
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-06-26 13:41:07 PDT
https://hg.mozilla.org/mozilla-central/rev/ca7394903dd3

Note You need to log in before you can comment on or make changes to this bug.