[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
11 months ago
9 months ago

People

(Reporter: sylvestre, Assigned: starsmarsjupitersaturn)

Tracking

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

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
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
(Reporter)

Comment 1

11 months ago
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]
(Assignee)

Comment 2

10 months ago
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)
(Reporter)

Comment 3

10 months ago
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)
(Assignee)

Comment 4

10 months ago
Yep, makes sense.  Let's see what I can do.
Priority: -- → P3
(Assignee)

Comment 5

10 months ago
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
(Assignee)

Comment 6

10 months ago
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.
(Reporter)

Comment 7

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

Updated

10 months ago
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'
(Assignee)

Comment 9

10 months ago
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.
(Assignee)

Comment 10

10 months ago
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

10 months ago
mozreview-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

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

Comment 12

10 months ago
mozreview-review-reply
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?
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
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

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

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

Comment 19

10 months ago
mozreview-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/#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.
(Assignee)

Comment 21

10 months ago
(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)

Comment 23

9 months ago
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

Comment 24

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/795f30107e2a
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Updated

9 months ago
You need to log in before you can comment on or make changes to this bug.