Closed Bug 1030299 Opened 5 years ago Closed 5 years ago

Cleanup a build warning in Volume.cpp

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dhylands, Assigned: reznord, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

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]
Hi,

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

Thanks in advance,

Regards,
A. Anup
Assignee: nobody → allamsetty.anup
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).
(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. :)
Attached patch Cleared the build warning. (obsolete) — Splinter Review
Added the changes according to the bug. Think I did it correctly. I tested it by building it after adding the changes.
Attachment #8446588 - Flags: review?(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.
Attachment #8446588 - Flags: review?(dhylands)
Made the changes according to the bug and created a patch.
Attachment #8446588 - Attachment is obsolete: true
Attachment #8446667 - Flags: review?(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]
Attachment #8446667 - Flags: review?(dhylands) → review-
Changed it according to the order in Volume.h and submitted a patch.
Attachment #8446667 - Attachment is obsolete: true
Attachment #8446996 - Flags: review?(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.
Attachment #8446996 - Flags: review?(dhylands) → review+
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.
Attachment #8446996 - Attachment is obsolete: true
Attachment #8447560 - Flags: review?(dhylands)
Attachment #8447560 - Flags: review?(dhylands) → review+
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.
Keywords: checkin-needed
Hi Anup, could you provide as Dave mentioned a try link for the checkin ?
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.
Keywords: checkin-needed
Here is the try request link:

https://tbpl.mozilla.org/?tree=Try&rev=7c06ae9b0509
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.
Keywords: checkin-needed
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
Keywords: checkin-needed
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/e5480588876b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.