Closed Bug 1466428 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: Sylvestre, Assigned: starsmarsjupitersaturn)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: