1106050 - Compile warning in xpcom/base/nsMemoryInfoDumper.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Zhenbo Li]

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

Comment 1

[Zhenbo Li]

Attachment: fix_compile.diff

My patch for this warning

Comment 2

[:Gijs (he/him)]

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?

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Zhenbo Li]

Attachment: DebugOnly.diff

I used DebugOnly<bool> in this patch

Comment 6

[Zhenbo Li]

Attachment: ifdef.diff

I used #ifdef in this patch, and I'm wondering that, this patch may have  multi-thread problems

Comment 7

[Eric Rahm [:erahm]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

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

Comment 9

[Zhenbo Li]

(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

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Zhenbo Li]

Attachment: DebugOnly_with_include.diff

I added `#include`

Comment 12

[Nicholas Nethercote [inactive]]

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.

Comment 13

[Zhenbo Li]

Attachment: FixStatic.diff

So glad that this problem has been spotted before sendding it to hg repo :)

Comment 14

[Zhenbo Li]

Try server result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=020939da8137
(I think this is enough for such a tiny patch)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9ea45b9e44

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e9ea45b9e44