Last Comment Bug 886322 - Remove keepAtoms member in TokenStream
: Remove keepAtoms member in TokenStream
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Andy Wingo [:wingo]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-24 08:41 PDT by Andy Wingo [:wingo]
Modified: 2013-08-01 14:00 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell (10.44 KB, patch)
2013-06-24 08:42 PDT, Andy Wingo [:wingo]
jorendorff: review+
Details | Diff | Splinter Review
Remove keepAtoms member in TokenStream (5.58 KB, patch)
2013-07-31 08:26 PDT, Andy Wingo [:wingo]
n.nethercote: review+
Details | Diff | Splinter Review

Description Andy Wingo [:wingo] 2013-06-24 08:41:37 PDT

    
Comment 1 Andy Wingo [:wingo] 2013-06-24 08:42:23 PDT
Created attachment 766710 [details] [diff] [review]
Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell
Comment 2 Andy Wingo [:wingo] 2013-06-24 08:43:34 PDT
Comment on attachment 766710 [details] [diff] [review]
Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell

It seems that positionAfterLastFunctionKeyword is useless since the introduction of lazy parsing.  Does this change make sense?  Seen while working on function*.
Comment 3 Andy Wingo [:wingo] 2013-06-24 08:44:27 PDT
Adding people from bug 819509.
Comment 4 Luke Wagner [:luke] 2013-06-24 09:12:30 PDT
Coincidentally, I just r+d one of bhackett's patches (lost the link to it now) that fixed a fuzz bug by doing exactly this.
Comment 5 Brian Hackett (:bhackett) 2013-06-24 09:37:39 PDT
Yeah, bug 884920 does this to fix a crash, should land today.
Comment 6 Andy Wingo [:wingo] 2013-06-24 09:54:59 PDT
Bizarre synchrony!  Note that this patch also removes the keepAtoms argument to TokenStream, as AFAICT it's no longer needed.  Not sure about that, though!
Comment 7 Jason Orendorff [:jorendorff] 2013-06-25 12:03:24 PDT
Comment on attachment 766710 [details] [diff] [review]
Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell

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

r=me to remove the keepAtoms argument, if that's what remains to be done here.
Comment 8 Andy Wingo [:wingo] 2013-07-31 08:26:37 PDT
Created attachment 783781 [details] [diff] [review]
Remove keepAtoms member in TokenStream
Comment 9 Andy Wingo [:wingo] 2013-07-31 08:28:31 PDT
Updated patch removes keepAtoms argument, setting checkin-needed.  Thanks for the review.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-07-31 14:23:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c432485f902
Comment 11 Nicholas Nethercote [:njn] 2013-07-31 15:45:35 PDT
Comment on attachment 783781 [details] [diff] [review]
Remove keepAtoms member in TokenStream

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

::: js/src/frontend/TokenStream.cpp
@@ +1168,5 @@
>              }
>              tt = TOK_NAME;
>              if (!checkForKeyword(chars, length, &tt))
>                  goto error;
> +            if (tt != TOK_NAME)                goto out;

This looks like a typo.  Please change it back!
Comment 12 Nicholas Nethercote [:njn] 2013-07-31 15:46:51 PDT
Comment on attachment 783781 [details] [diff] [review]
Remove keepAtoms member in TokenStream

Also, if you post an updated version of an r+'d patch for check-in, it's customary to mark it r+ yourself.  That way, the non-obsoleted patches all have r+ on them, which saves people thinking "why was that non-r+'d patch checked in?"
Comment 13 Andy Wingo [:wingo] 2013-08-01 04:01:16 PDT
(In reply to Nicholas Nethercote [:njn] from comment #11)
> Comment on attachment 783781 [details] [diff] [review]
> Remove keepAtoms member in TokenStream
> 
> Review of attachment 783781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/TokenStream.cpp
> @@ +1168,5 @@
> >              }
> >              tt = TOK_NAME;
> >              if (!checkForKeyword(chars, length, &tt))
> >                  goto error;
> > +            if (tt != TOK_NAME)                goto out;
> 
> This looks like a typo.  Please change it back!

Sorry about that.  Good catch!  Will fold into bug 885695, I guess.
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-08-01 14:00:11 PDT
https://hg.mozilla.org/mozilla-central/rev/5c432485f902

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