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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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'

Comment 1

[Umesh Panchaksharaiah]

Hi, request to assign this bug to me. I have another similar bug.
I can fix this one as well. Thanks

Comment 2

[Sylvestre Ledru [:Sylvestre]]

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

Comment 3

[Umesh Panchaksharaiah]

Fair enough. Having someone else fix this to understand the workflow 
makes sense. Thanks.

Comment 4

[pass2pawan]

Hello!
I just started working on bugs here, Can someone take me through the process of taking this bug and fixing it ?

Comment 5

[Umesh Panchaksharaiah]

This bug is already assigned and being worked on. Thanks.

Comment 6

[Umesh Panchaksharaiah]

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

Comment 7

[pass2pawan]

@umesh are there any ways other than installing the build ?

Comment 8

[Umesh Panchaksharaiah]

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.

Comment 9

[Kanika Narang[:kxnikx]]

Hii, I am new here.I would like to work on this bug.Can anyone tell me how to proceed ?

Comment 10

[Kirti Singla]

Hi, I am new here and I request to assign this bug to me.

Comment 11

[Kanika Narang[:kxnikx]]

Is anyone working on this bug ?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

I will assign it to the person who provides a patch

Comment 13

[pass2pawan]

@sylvestre how do we go about doing that exactly?

Comment 14

[Kanika Narang[:kxnikx]]

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 ?

Comment 15

[Kanika Narang[:kxnikx]]

Attachment: Bug 1301627 - changed the name of variables in the comment on line 521.

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

Comment 16

[Kirti Singla]

Attachment: Changed the names of variables in the comment line 521

Comment 17

[Kirti Singla]

Attachment: changed the name of variables in the comment

Comment 18

[Tooru Fujisawa [:arai]]

review flag is not set on your latest patch.
if the patch is ready, can you set review flag?

Comment 19

[Tooru Fujisawa [:arai]]

thanks :)

Comment 20

[Sylvestre Ledru [:Sylvestre]]

This looks good but I am not the component owner. You will have to find him/her! :)

Comment 21

[Kirti Singla]

I am a new mozilla contributor so can u guide me through further steps.

Comment 22

[Tooru Fujisawa [:arai]]

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.

Comment 23

[Eric Rahm [: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

Comment 24

[Kirti Singla]

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.

Comment 25

[Kirti Singla]

Attachment: Changed the name of variables in the comment on line 521

Comment 26

[Kirti Singla]

Attachment: Bug 1301627 - Changed the name of variables in the comment on line 521

Comment 27

[Eric Rahm [: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.

Comment 28

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93cdd187a2535349195fd5d4ec71bee88fa04f54
Bug 1301627 - Change the name of commented variables when calling perf_event_open. r=erahm

Comment 29

[Eric Rahm [: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!

Comment 30

[Kirti Singla]

(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.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93cdd187a253