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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Aaron]

Hi, I am new here and would like to fix this bug. Could you tell me how to proceed?

Comment 2

[Tooru Fujisawa [:arai]]

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

Comment 3

[vinayakagarwal6996]

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!

Comment 4

[Tooru Fujisawa [:arai]]

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.

Comment 5

[vinayakagarwal6996]

Attachment: Used consumeKnownToken followed by the required if statement

Please review the patch.
Thanks!

Comment 6

[Tooru Fujisawa [:arai]]

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 "&".

Comment 7

[Tooru Fujisawa [:arai]]

Aaron, if you're looking for C++ bug, I've filed bug 1304310, feel free to take it.

Comment 8

[vinayakagarwal6996]

Attachment: used consumeKnownToken(tt)

Please review the commit. I built it and it gave no errors

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8793460 [details] [diff] [review]
used consumeKnownToken(tt)

please check "patch" when attaching ;)

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8793175 [details] [diff] [review]
Used consumeKnownToken followed by the required if statement

and also mark previous patch "obsolete"

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8793460 [details] [diff] [review]
used consumeKnownToken(tt)

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

perfect :)

Comment 12

[vinayakagarwal6996]

(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!

Comment 13

[Tooru Fujisawa [:arai]]

in attachment form, above the comment field.

Comment 14

[Tooru Fujisawa [:arai]]

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)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8d7c4926fb
Used consumeKnownToken instead of getToken. r=arai

Comment 16

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/ff8d7c4926fb