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

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
trivial
RESOLVED FIXED
Last year
Last year

People

(Reporter: sylvestre, Assigned: starsmarsjupitersaturn)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [good first bug][lang=C++, Rust])

Attachments

(2 attachments)

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
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.
Whiteboard: [good first bug][lang=C++] → [good first bug][lang=C++, Rust]
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
Flags: needinfo?(sledru)
Sounds good! :)
You should write a patch to update the generator and the second patch to update the code. Makes sense?
Assignee: nobody → starsmarsjupitersaturn
Flags: needinfo?(sledru)
Yep, makes sense.  Let's see what I can do.
Priority: -- → P3
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
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.
Building locally will probably be enough. You should just share your patch :)
Thanks
Summary: BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX' → [BinAST] BinSource-auto.cpp: argument name 'YYY' in comment does not match parameter name 'XXX'
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.
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 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.
Attachment #8990355 - Flags: review+
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 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 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.
Sorry for double comment.  It was something that appeared wrong in my terminal.
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!
Attachment #8990355 - Flags: review+
(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.
(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.
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.
Flags: needinfo?(starsmarsjupitersaturn)
Flags: needinfo?(starsmarsjupitersaturn)
Pushed by arai_a@mac.com:
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
https://hg.mozilla.org/mozilla-central/rev/795f30107e2a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.