1030299 - Cleanup a build warning in Volume.cpp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

I noticed the following build warnings, which should be fixed:

> In file included from ../../../../b2g-inbound/dom/system/gonk/Volume.cpp:5:0:
> ../../../../b2g-inbound/dom/system/gonk/Volume.h: In constructor 'mozilla::system::Volume::Volume(const nsCSubstring&)':
> ../../../../b2g-inbound/dom/system/gonk/Volume.h:111:8: warning: 'mozilla::system::Volume::mIsSharing' will be initialized after [-Wreorder]
> ../../../../b2g-inbound/dom/system/gonk/Volume.h:107:8: warning:   'bool mozilla::system::Volume::mFormatRequested' [-Wreorder]
> ../../../../b2g-inbound/dom/system/gonk/Volume.cpp:55:1: warning:   when initialized here [-Wreorder]

Comment 1

[Anup Kumar [:reznord]]

Hi,

I am interested in working on this bug. Can you please assign it to me?

Thanks in advance,

Regards,
A. Anup

Comment 2

[Dave Hylands [:dhylands]]

It is now assigned to you. You'll need to be able to build B2G. Let me know if I can help. I'm in the Pacific Time Zone (UTC-7).

Comment 3

[Anup Kumar [:reznord]]

(In reply to Dave Hylands [:dhylands] from comment #2)
> It is now assigned to you. You'll need to be able to build B2G. Let me know
> if I can help. I'm in the Pacific Time Zone (UTC-7).

I have completed the B2G build in my system without any errors. It works perfectly fine. :)

Comment 4

[Anup Kumar [:reznord]]

Attachment: Cleared the build warning.

Added the changes according to the bug. Think I did it correctly. I tested it by building it after adding the changes.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8446588 [details] [diff] [review]
Cleared the build warning.

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

So your patch contains both the changes in Volume.cpp and a patch of the changes in Volume.cpp.

You should remove the patch within a patch, since we don't want to check that in.

Comment 6

[Anup Kumar [:reznord]]

Attachment: Cleared the build warning in Volume.cpp

Made the changes according to the bug and created a patch.

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8446667 [details] [diff] [review]
Cleared the build warning in Volume.cpp

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

Hmm. So after applying your patch, I now get the following warning instead:

> In file included from ../../../../b2g-inbound/dom/system/gonk/Volume.cpp:5:0:
> ../../../../b2g-inbound/dom/system/gonk/Volume.h: In constructor 'mozilla::system::Volume::Volume(const nsCSubstring&)':
> ../../../../b2g-inbound/dom/system/gonk/Volume.h:110:8: warning: 'mozilla::system::Volume::mCanBeShared' will be initialized after [-Wreorder]
> ../../../../b2g-inbound/dom/system/gonk/Volume.h:107:8: warning:   'bool mozilla::system::Volume::mFormatRequested' [-Wreorder]
> ../../../../b2g-inbound/dom/system/gonk/Volume.cpp:55:1: warning:   when initialized here [-Wreorder]

Comment 8

[Anup Kumar [:reznord]]

Attachment: Changed the order according to Volume.h and removed the warnings.

Changed it according to the order in Volume.h and submitted a patch.

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 8446996 [details] [diff] [review]
Changed the order according to Volume.h and removed the warnings.

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

Excellent - no more warnings.

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 8446996 [details] [diff] [review]
Changed the order according to Volume.h and removed the warnings.

The subject shouldn't have [PATCH] in front of the bug number, but otherwise this looks good.

So please edit the patch and re-attach it.

Comment 11

[Anup Kumar [:reznord]]

Attachment: removed the [patch] from the subject.

Comment 12

[Dave Hylands [:dhylands]]

OK - next step is to get it landed. Since you don't have checkin priviledge yet, you need to add "checkin-needed" to the keywords on the bug.

However, the current policy requires a try-run before the sherriffs will land any checkin-needed bugs.

In order to submit a try-run you'll need to get Level 1 committer access.

http://www.mozilla.org/hacking/commit-access-policy/
https://wiki.mozilla.org/ReleaseEngineering/TryServer

I'd be happy to vouch for you.

Feel free to ask if you have any questions.

Comment 13

[Carsten Book [:Tomcat]]

Hi Anup, could you provide as Dave mentioned a try link for the checkin ?

Comment 14

[Dave Hylands [:dhylands]]

Anup - I'm going to remove checkin-needed for now. Once you've got the try-run happening, then we can add it back then.

Comment 15

[Anup Kumar [:reznord]]

Here is the try request link:

https://tbpl.mozilla.org/?tree=Try&rev=7c06ae9b0509

Comment 16

[Dave Hylands [:dhylands]]

That looks good. This particular bug only modified files which get built by B2G - and those were all green.

So I added checkin-needed back to the bug.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5480588876b

Please try to be more conscientious about what builds/tests you run on your Try pushes. For something like this, a simple "does it build?" push on one platform probably would have sufficed instead of running all builds and tests on every platform. Doing so wastes tons of machine time (300+ hours) and contributes to long backlogs felt by everybody else. The link below provides some more recommendations about using Try. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 18

[Anup Kumar [:reznord]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e5480588876b
> 
> Please try to be more conscientious about what builds/tests you run on your
> Try pushes. For something like this, a simple "does it build?" push on one
> platform probably would have sufficed instead of running all builds and
> tests on every platform. Doing so wastes tons of machine time (300+ hours)
> and contributes to long backlogs felt by everybody else. The link below
> provides some more recommendations about using Try. Thanks!

I am really sorry for my mistake. From the next time I will make sure that it won't happen again.
Once again I am really sorry for what happened.

Comment 19

[Dave Hylands [:dhylands]]

That was really my mistake. I suggested to Anup what try string to use, and since this was his first try run, I was trying to make things simple and not overcomplicate.

In this particular case we probably could have gotten away with a build-only limited to B2G build.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5480588876b