Closed Bug 1106050 Opened 5 years ago Closed 5 years ago

Compile warning in xpcom/base/nsMemoryInfoDumper.cpp

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: litimetal, Assigned: litimetal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141113113219

Steps to reproduce:

./mach build


Actual results:

warning: variable ‘fifoCallbacksRegistered’ set but not used [-Wunused-but-set-variable]
 8:41.23    static bool fifoCallbacksRegistered = false;
 8:41.23                ^



Expected results:

Compile without warnings
Attached patch fix_compile.diff (obsolete) — Splinter Review
My patch for this warning
Attachment #8530271 - Flags: review?(n.nethercote)
But the variable is used here, isn't it?

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryInfoDumper.cpp#230

Against what tree and what revision did you build?
Component: Untriaged → XPCOM
Flags: needinfo?(litimetal)
Product: Firefox → Core
Comment on attachment 8530271 [details] [diff] [review]
fix_compile.diff

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

Thank you for working on this. It's good to get rid of warnings -- see bug 187528 and its dependent bugs!

The variable is used in the MOZ_ASSERT, which gets removed in non-debug builds but *is* used in debug builds. So the right thing would be to put the variable declaration and assignment within |#ifdef DEBUG| conditions.

Furthermore, it looks like you should be able to move three statements that use this variable in a row, just after the MaybeCreate() block, which would allow you to have just one |#ifdef DEBUG| condition instead of two.
Attachment #8530271 - Flags: review?(n.nethercote)
You should be able to change the type to something like static DebugOnly<bool> and then it will be defined in debug builds and ignored otherwise.
Attached patch DebugOnly.diff (obsolete) — Splinter Review
I used DebugOnly<bool> in this patch
Attachment #8530271 - Attachment is obsolete: true
Flags: needinfo?(litimetal)
Attachment #8530618 - Flags: review?(n.nethercote)
Attached patch ifdef.diff (obsolete) — Splinter Review
I used #ifdef in this patch, and I'm wondering that, this patch may have  multi-thread problems
Attachment #8530621 - Flags: review?(n.nethercote)
Comment on attachment 8530618 [details] [diff] [review]
DebugOnly.diff

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

|DebugOnly<T>| is probably the cleanest solution. I think you'll also want to include mfbt/DebugOnly.h as well.
Attachment #8530618 - Flags: feedback+
(In reply to Eric Rahm [:erahm] from comment #7)
> |DebugOnly<T>| is probably the cleanest solution. I think you'll also want
> to include mfbt/DebugOnly.h as well.

To be precise, it is actually:
#include "mozilla/DebugOnly.h"
Assignee: nobody → litimetal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8530618 - Flags: review?(n.nethercote) → review+
(In reply to Andrew McCreight [:mccr8] from comment #8)
> (In reply to Eric Rahm [:erahm] from comment #7)
> > |DebugOnly<T>| is probably the cleanest solution. I think you'll also want
> > to include mfbt/DebugOnly.h as well.
> 
> To be precise, it is actually:
> #include "mozilla/DebugOnly.h"

Thanks, Eric

I tried 
#ifdef mozilla_DebugOnly_h
And found that DebugOnly.h has been included indirectly.

Should I include DebugOnly.h explicitly?
Thank you very much
> Should I include DebugOnly.h explicitly?

Yes please. It's considered good practice. If you don't, it's possible that compilation will break in the future if the file through which the indirect inclusion occurs is removed from this file's #include list.
Attachment #8530621 - Flags: review?(n.nethercote)
Attached patch DebugOnly_with_include.diff (obsolete) — Splinter Review
I added `#include`
Attachment #8530618 - Attachment is obsolete: true
Attachment #8530621 - Attachment is obsolete: true
Attachment #8531829 - Flags: review?(n.nethercote)
Comment on attachment 8531829 [details] [diff] [review]
DebugOnly_with_include.diff

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

Oh! The variable must stay |static|, because it's crucial that its value persists across multiple calls to the function so that the setup only happens once. Sorry, I should have caught that on the previous review.
Attachment #8531829 - Flags: review?(n.nethercote) → review-
Attached patch FixStatic.diffSplinter Review
So glad that this problem has been spotted before sendding it to hg repo :)
Attachment #8531829 - Attachment is obsolete: true
Attachment #8532020 - Flags: review?(n.nethercote)
Attachment #8532020 - Flags: review?(n.nethercote) → review+
Try server result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=020939da8137
(I think this is enough for such a tiny patch)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e9ea45b9e44
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.