Compile warning in xpcom/base/nsMemoryInfoDumper.cpp

RESOLVED FIXED in mozilla37

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Zhenbo Li, Assigned: Zhenbo Li)

Tracking

(Blocks: 1 bug)

Trunk
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8530271 [details] [diff] [review]
fix_compile.diff

My patch for this warning

Updated

4 years ago
Attachment #8530271 - Flags: review?(n.nethercote)

Comment 2

4 years ago
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)
Blocks: 187528
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8530618 [details] [diff] [review]
DebugOnly.diff

I used DebugOnly<bool> in this patch
Attachment #8530271 - Attachment is obsolete: true
Flags: needinfo?(litimetal)
Attachment #8530618 - Flags: review?(n.nethercote)
(Assignee)

Comment 6

4 years ago
Created attachment 8530621 [details] [diff] [review]
ifdef.diff

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+
(Assignee)

Comment 9

4 years ago
(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)
(Assignee)

Comment 11

4 years ago
Created attachment 8531829 [details] [diff] [review]
DebugOnly_with_include.diff

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-
(Assignee)

Comment 13

4 years ago
Created attachment 8532020 [details] [diff] [review]
FixStatic.diff

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+
(Assignee)

Comment 14

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.