Closed Bug 886322 Opened 6 years ago Closed 6 years ago

Remove keepAtoms member in TokenStream

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: wingo, Assigned: wingo)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: general → wingo
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*.
Attachment #766710 - Flags: review?(jorendorff)
Adding people from bug 819509.
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.
Yeah, bug 884920 does this to fix a crash, should land today.
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 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.
Attachment #766710 - Flags: review?(jorendorff) → review+
Attachment #766710 - Attachment is obsolete: true
Updated patch removes keepAtoms argument, setting checkin-needed.  Thanks for the review.
Keywords: checkin-needed
Summary: Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell → Remove keepAtoms member in TokenStream
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 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?"
Attachment #783781 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5c432485f902
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.