Closed Bug 1304097 Opened 4 years ago Closed 4 years ago

Use TokenStream::consumeKnownToken instead of TokenStream::getToken for known token.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: vinayakagarwal6996, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 1 obsolete file)

https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/src/frontend/Parser.cpp#4531
>             if (!tokenStream.peekToken(&tt))
>                 return null();
> 
>             if (tt == TOK_COMMA) {
>                 if (!tokenStream.getToken(&tt) || !tokenStream.getToken(&tt))
>                     return null();

the first |tokenStream.getToken(&tt)| gets the token that is peeked by |tokenStream.peekToken(&tt)| above, and it's known to be TOK_COMMA.
We should use |tokenStream.consumeKnownToken(tt);| instead.
Hi, I am new here and would like to fix this bug. Could you tell me how to proceed?
Hi :)

Here's documents explaining workflow.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html

Basically, clone the tree, fix the bug, make it a patch and post, and ask review.

If you have any question, feel free to ask here or in IRC #introduction channel.
https://wiki.mozilla.org/IRC
Hello,
I am a first timer. I looked up consumeKnownToken here:
https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.h

According to this, consumeKnownToken returns void. 
So if it is in an if statement, it will give an error.

Please correct me where I am wrong

Thanks!
You can move it out of if's condition.
consumeKnownToken never fails, unless it's called wrongly, like the passed TokenKind doesn't match to the token in lookahead buffer.
and if it fails, it intentionally crashes (on debug build), instead of returning falsy value.
Please review the patch.
Thanks!
Attachment #8793175 - Flags: review?(arai.unmht)
Comment on attachment 8793175 [details] [diff] [review]
Used consumeKnownToken followed by the required if statement

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

thank you for your patch :)

it would be nice to build and test patch locally, before attaching.

here's build documentation for
  SpiderMonkey shell:
    https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
  whole Firefox:
    https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

::: js/src/frontend/Parser.cpp
@@ +4528,4 @@
>                  return null();
>  
>              if (tt == TOK_COMMA) {
> +            	tokenStream.consumeKnownToken(&tt);

consumeKnownToken receives TokenKind, not TokenKind*.
so you don't need "&".
Attachment #8793175 - Flags: review?(arai.unmht) → feedback+
Aaron, if you're looking for C++ bug, I've filed bug 1304310, feel free to take it.
Please review the commit. I built it and it gave no errors
Attachment #8793460 - Flags: review?(arai.unmht)
Comment on attachment 8793460 [details] [diff] [review]
used consumeKnownToken(tt)

please check "patch" when attaching ;)
Attachment #8793460 - Attachment is patch: true
Comment on attachment 8793175 [details] [diff] [review]
Used consumeKnownToken followed by the required if statement

and also mark previous patch "obsolete"
Attachment #8793175 - Attachment is obsolete: true
Assignee: nobody → vinayakagarwal6996
Status: NEW → ASSIGNED
Comment on attachment 8793460 [details] [diff] [review]
used consumeKnownToken(tt)

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

perfect :)
Attachment #8793460 - Flags: review?(arai.unmht) → review+
(In reply to Tooru Fujisawa [:arai] from comment #10)
> Comment on attachment 8793175 [details] [diff] [review]
> Used consumeKnownToken followed by the required if statement
> 
> and also mark previous patch "obsolete"

Where is the option to mark the patch obselete?
I couldn't see it.
Thanks!
in attachment form, above the comment field.
here's try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda2e532946c

(your changeset is not shown there but it's there in parent of the changeset)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8d7c4926fb
Used consumeKnownToken instead of getToken. r=arai
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff8d7c4926fb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.