Closed Bug 1301627 Opened 3 years ago Closed 3 years ago

tools/power/* comments don't match the parameter names

Categories

(Core :: General, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Sylvestre, Assigned: singlakirti3, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 3 obsolete files)

This is a trivial bug. This has been found by clang-tidy ( http://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html ).
The idea is to give an easy bug for someone who wants to understand how to contribute to Firefox.

tools/power/rapl.cpp
@ Line 443	argument name 'cpu' in comment does not match parameter name 'aCpu'

tools/power/rapl.cpp
@ Line 443	argument name 'pid' in comment does not match parameter name 'aPid'

tools/power/rapl.cpp
@ Line 444	argument name 'flags' in comment does not match parameter name 'aFlags'

tools/power/rapl.cpp
@ Line 444	argument name 'group_fd' in comment does not match parameter name 'aGroupFd'
Summary: tools/power/* Comments doesn't match the parameter names → tools/power/* comments don't match the parameter names
Hi, request to assign this bug to me. I have another similar bug.
I can fix this one as well. Thanks
As it is a bug to understand our workflow and it is not about programming, I would prefer to leave this one for someone else. 
Sorry
Fair enough. Having someone else fix this to understand the workflow 
makes sense. Thanks.
Hello!
I just started working on bugs here, Can someone take me through the process of taking this bug and fixing it ?
This bug is already assigned and being worked on. Thanks.
My apologies. Wrong update. This bug is available. Please go ahead.

I would recommend installing the build vm:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Using_the_VM#Working_in_the_VM

Please have a look at the developer guide:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
@umesh are there any ways other than installing the build ?
I am a newbie and so far I have been using the VM.
I guess you need an environment to do the build to change code.
Hii, I am new here.I would like to work on this bug.Can anyone tell me how to proceed ?
Hi, I am new here and I request to assign this bug to me.
Is anyone working on this bug ?
I will assign it to the person who provides a patch
@sylvestre how do we go about doing that exactly?
I was trying to submit a patch on mozreview but it is showing me this error 
abort: cannot submit reviews referencing multiple bugs
(limit reviewed changesets with "-c" or "-r" arguments)
can you help me with that ?
Attachment #8793851 - Flags: review?(sledru)
Attachment #8793851 - Attachment is obsolete: true
Attachment #8793851 - Flags: review?(sledru)
review flag is not set on your latest patch.
if the patch is ready, can you set review flag?
Flags: needinfo?(singlakirti3)
Attachment #8793852 - Flags: review?(sledru)
thanks :)
Flags: needinfo?(singlakirti3)
This looks good but I am not the component owner. You will have to find him/her! :)
Assignee: nobody → singlakirti3
Attachment #8793852 - Flags: review?(sledru) → feedback+
I am a new mozilla contributor so can u guide me through further steps.
you can ask a review from someone who has reviewed the file several times, or who reviewed the part you've touched.

you can check reviewer in commit log for the file.
https://hg.mozilla.org/mozilla-central/filelog/f0f1aaf051d6798e1e73d1feee07ca847333167a/tools/power/rapl.cpp
or hg annotate
https://hg.mozilla.org/mozilla-central/annotate/f0f1aaf051d6798e1e73d1feee07ca847333167a/tools/power/rapl.cpp

in this case, erahm will be suitable reviewer.
Attachment #8793852 - Flags: review?(erahm)
Comment on attachment 8793852 [details] [diff] [review]
changed the name of variables in the comment

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

This is a great start, I'm going to clear the review request for now. Can you update the param names and add a new patch with the changes?

::: tools/power/rapl.cpp
@@ +518,5 @@
>      attr.config = config;
>  
>      // Measure all processes/threads. The specified CPU doesn't matter.
> +    mFd = perf_event_open(&attr, /* apid = */ -1, /* acpu = */ 0,
> +                          /* agroup_fd = */ -1, /* aflags = */ 0);

I think the tool wants you to use the exact param names, you can see those in the |perf_event_open| definition [1] (aPid, aCpu, etc).

[1] https://dxr.mozilla.org/mozilla-central/rev/f0f1aaf051d6798e1e73d1feee07ca847333167a/tools/power/rapl.cpp#443
Attachment #8793852 - Flags: review?(erahm)
Due to system failure I need to rebuild firefox on my machine, so I will upload the new patch as soon as my machine is up and running again.
Attachment #8803713 - Attachment is obsolete: true
Attachment #8803713 - Attachment is patch: false
Attachment #8793852 - Attachment is obsolete: true
Attachment #8803715 - Flags: review?(erahm)
Comment on attachment 8803715 [details] [diff] [review]
Bug 1301627 - Changed the name of variables in the comment on line 521

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

This looks good, thank you for your contribution! I'll go ahead and land it for you.
Attachment #8803715 - Flags: review?(erahm) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/93cdd187a2535349195fd5d4ec71bee88fa04f54
Bug 1301627 - Change the name of commented variables when calling perf_event_open. r=erahm
Kirti, I landed the change for you with an updated commit message. We like to have the name of the reviewer added to the commit message (the "r=erahm" part) and a made the message a bit more generic. I look forward to seeing your next patch!
(In reply to Eric Rahm [:erahm] from comment #29)
> Kirti, I landed the change for you with an updated commit message. We like
> to have the name of the reviewer added to the commit message (the "r=erahm"
> part) and a made the message a bit more generic. I look forward to seeing
> your next patch!

Thanks a lot for your help and support. I wanted to know if the work on this bug is complete or is there something more to be done.
https://hg.mozilla.org/mozilla-central/rev/93cdd187a253
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.