Remove keepAtoms member in TokenStream

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, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 766710 [details] [diff] [review]
Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell
(Assignee)

Updated

4 years ago
Assignee: general → wingo
(Assignee)

Comment 2

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

Comment 3

4 years ago
Adding people from bug 819509.

Comment 4

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

Comment 6

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

Comment 8

4 years ago
Created attachment 783781 [details] [diff] [review]
Remove keepAtoms member in TokenStream
(Assignee)

Updated

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

Comment 9

4 years ago
Updated patch removes keepAtoms argument, setting checkin-needed.  Thanks for the review.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Summary: Remove TokenStream::positionAfterLastFunctionKeyword in favor of TokenStream::tell → Remove keepAtoms member in TokenStream
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c432485f902
Keywords: checkin-needed
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+
(Assignee)

Comment 13

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.