1466428 - [BinAST] BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

Reporting this trivial change as good first bug for people who would like to understand the contribution process.

All in the same file: BinSource-auto.cpp

Line 6100
argument name 'finally' in comment does not match parameter name 'finallyBlock'

Lines 6988 6988
argument name 'item' in comment does not match parameter name 'kid'

Line 6843
argument name 'child' in comment does not match parameter name 'kid'

More information about the checkers:
https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Actually, this file is generated by some rust code (making the line number incorrect). It isn't that trivial but not very hard either.

As steps:
* Found the generator
* Find the source input file (hint: it is a yaml file)
* Find the lines
* Update the source
* Restart the generator to check it is working.

Comment 2

[starsmarsjupitersaturn]

Hi there,

I am new here.  I have been researching this for a couple days, and looked at the BinSource-auto.cpp file, BinSource.yaml and I think I found where the changes need to happen.  @sylvestre says that 'finally', 'item', and 'child' are comments that need to be changed.  I found them in src/js/frontend/BinSource.yaml  According to my research, there is literally only one result (from searching in my text editor) for each of these things I am looking for.

On line 948 of BinSource.yaml, I found:
        BINJS_TRY_DECL(result, factory_.newTryStatement(start, body, catchClause, /* finally = */ nullptr));

On line 734 of BinSource.yaml, I found:
        factory_.addList(/* list = */ result, /* item = */ item);

On line 199 of BinSource.yaml, I found:
        factory_.addList(/* list = */ result, /* child = */ item);

After I make these changes to these comments, should I run the rust generator (and then test the code locally of course)..?  I found a README in js/src/frontend/binsource/README.md that says how to run the generator.  I will need to run it locally again as it failed the first time giving me a file not found error, which I will look into locally on my machine further..  However, I read in documentation elsewhere for SpiderMonkey that I can build it from there as well.  Just wanted to go over all this ahead of time especially since it's my first time here, hope you didn't mind the jibber jabber.

Thanks

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Sounds good! :)
You should write a patch to update the generator and the second patch to update the code. Makes sense?

Comment 4

[starsmarsjupitersaturn]

Yep, makes sense.  Let's see what I can do.

Comment 5

[starsmarsjupitersaturn]

Ok, for this bug I have found a typo in the README.md for running the generator which will I will need to go through the process of creating another bug for that.  Also, I am going to request level 1 commit access on the try server and try to get the code up there.  I have the code locally.  Thx

Comment 6

[starsmarsjupitersaturn]

Here is the bug I created to request access, hope I can put this here.  https://bugzilla.mozilla.org/show_bug.cgi?id=1473770

Once I have commit access, i'll try to send it to the try server and check the automated tests, and submit to mozreview.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Building locally will probably be enough. You should just share your patch :)
Thanks

Comment 8

[starsmarsjupitersaturn]

Attachment: Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'. Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

Review commit: https://reviewboard.mozilla.org/r/255412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255412/

Comment 9

[starsmarsjupitersaturn]

Ok, I submitted the patch for MozReview.  I will look into creating the bug for the typo in the README.md.

When I was rebasing with hg rebase before submitted to MozReview, It gave me some merge problems on other parts of BinSource-auto.cpp.  In the source tree version, it has extra returns between some of the lines of code.  In my version, it did not.  I wish I could've saved a screenshot of it to show you exactly what I mean and save you time.

Comment 10

[starsmarsjupitersaturn]

Probably a linting problem that I didn't do on my part.  If so, let me know, although,since I discarded any other changes than my own, it might not be a problem particularly.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

https://reviewboard.mozilla.org/r/255412/#review262242

Thank you!

to re-generate the BinSource-auto.cpp, please install rust nightly, and then run build.sh in js/src/frontend/binsource/ directory.

Comment 12

[starsmarsjupitersaturn]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

https://reviewboard.mozilla.org/r/255412/#review262242

Hi Tooru,

I ran the generator again after installing rust's latest 1.27.0 released jun 21 2018.  I see three changes in the diff.  So are those the ones I need to commit again?

Comment 13

[starsmarsjupitersaturn]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

https://reviewboard.mozilla.org/r/255412/#review262242

And thank you as well :)

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

https://reviewboard.mozilla.org/r/255412/#review262242

great :)
"hg commit --amend" stores the diff into the current changeset, and then you can push it to review repository again to update.

Comment 15

[starsmarsjupitersaturn]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255412/diff/1-2/

Comment 16

[starsmarsjupitersaturn]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255412/diff/2-3/

Comment 17

[starsmarsjupitersaturn]

Sorry for double comment.  It was something that appeared wrong in my terminal.

Comment 18

[starsmarsjupitersaturn]

Attachment: 2018-07-06_19-07-22.png However I get this image when trying to run nightly.

Comment 19

[Tooru Fujisawa [:arai]]

Comment on attachment 8990355 [details]
Bug 1466428 - BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct gener

https://reviewboard.mozilla.org/r/255412/#review262364

excellent!
thank you!

Comment 20

[Tooru Fujisawa [:arai]]

(In reply to starsmarsjupitersaturn from comment #18)
> Created attachment 8990485 [details]
> 2018-07-06_19-07-22.png However I get this image when trying to run nightly.

sounds like build doesn't completely finish, or maybe something goes wrong in the object directory ("obj-*" inside mozilla-central).
can you try running "./mach build" again and see what it says?
also, if you're using non-native file system for storing mozilla-central repository or the object directory,
it's highly likely cause some trouble.

Comment 21

[starsmarsjupitersaturn]

(In reply to Tooru Fujisawa [:arai] from comment #20)
> (In reply to starsmarsjupitersaturn from comment #18)
> > Created attachment 8990485 [details]
> > 2018-07-06_19-07-22.png However I get this image when trying to run nightly.
> 
> sounds like build doesn't completely finish, or maybe something goes wrong
> in the object directory ("obj-*" inside mozilla-central).
> can you try running "./mach build" again and see what it says?
> also, if you're using non-native file system for storing mozilla-central
> repository or the object directory,
> it's highly likely cause some trouble.

Thanks, that was probably it.

Comment 22

[Tooru Fujisawa [:arai]]

can you trigger try in reviewboard?
there's "Automation" dropdown, and "Trigger a Try Build" inside that menu.
I think the following trychooser syntax should be fine, which triggers plain SpiderMonkey build and test.
  try: -b do -p sm-plain-linux64 -u none -t none
if it doesn't work, let me know.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/795f30107e2a
BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'.  Auto generated by rust generator and the yaml file, these comments were pointing to old parameters which have now been updated to have the correct generated source file. r=arai

Comment 24

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/795f30107e2a