Closed Bug 1010103 Opened 7 years ago Closed 7 years ago

clang warning on Linux: "nsParentalControlsService.h:34:8: warning: private field 'mEnabled' is not used [-Wunused-private-field]"


(Toolkit :: General, defect)

Not set





(Reporter: dholbert, Assigned: Six)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

After Bug 1006051's landing, I now get this build warning on Linux, using clang:
In file included from $SRCDIR/toolkit/components/parentalcontrols/nsParentalControlsServiceDefault.cpp:6:

Warning: -Wunused-private-field in $SRCDIR/toolkit/components/parentalcontrols/nsParentalControlsService.h: private field 'mEnabled' is not used

$SRCDIR/toolkit/components/parentalcontrols/nsParentalControlsService.h:34:8: warning: private field 'mEnabled' is not used [-Wunused-private-field]

  bool mEnabled;
This is just the default "dummy" implementation of this service, which does nothing (and which is the implementation that we use on Linux). The "real" implementations (on other platforms) do use this variable, which is why it's there.

I think we should make nsParentalControlsServiceDefault.cpp initialize this variable and then pass it to "mozilla::unused".

(The initialization is technically unnecessary but probably needed to avoid triggering an uninitialized variable warnings/issues when passing it to unused. Seems like the exact value doesn't really matter, since the variable is unused in this class anyway.)
This patch is based on the discussion we add with Dholbert and the explanation he wrote in his last comment.
Assignee: nobody → six.dsn
Attachment #8480931 - Flags: review?(dougt)
I think we need to initialize it (to false) in the constructor init list, like we do for Android here:

Otherwise, we'll be "using" an uninitialized value, when we pass it to unused<<, which isn't really kosher.
Flags: needinfo?(six.dsn)
This version is seeting mEnabled as false.
Attachment #8480931 - Attachment is obsolete: true
Attachment #8480931 - Flags: review?(dougt)
Attachment #8481135 - Flags: review?(dougt)
Flags: needinfo?(six.dsn)
Comment on attachment 8481135 [details] [diff] [review]

I'm going to see if I can get Doug's attention via IRC, but two nits that I just noticed when skimming the patch:

>Bug 1010103: Remove Linux Clang Warning on unused private field mEnabled in nsParentalControlsService.cpp r=dougt

s/Remove/Suppress/ perhaps?

("Remove warning" sounds odd to me -- that sounds almost like we're editing a compiler to remove its support for a warning, or something. :))

>diff --git a/toolkit/components/parentalcontrols/nsParentalControlsServiceDefault.cpp b/toolkit/components/parentalcontrols/nsParentalControlsServiceDefault.cpp
>+nsParentalControlsService::nsParentalControlsService() :
> {
>+  mozilla::unused << mEnabled;

The "mEnabled(false)" there needs to be indented by 2 spaces. It should look like the other nsParentalControlsService implementations:
Attachment #8481135 - Flags: feedback+
This is a new version including dholbert's feedbacks (Thanks btw)
Attachment #8481135 - Attachment is obsolete: true
Attachment #8481135 - Flags: review?(dougt)
Attachment #8489849 - Flags: review?(dougt)

dougt, can you either take a look at this or suggest someone else who can review it? (It's pretty trivial... See comment 1 for [minor] background on this class, to refresh your memory, if you like)
Flags: needinfo?(dougt)
Blocks: 1068977
Smaug said in IRC that he'd be up for r+'ing this.
Flags: needinfo?(dougt) → needinfo?(bugs)
Attachment #8489849 - Flags: review?(dougt) → review+
Thanks, jaws!
Flags: needinfo?(bugs)
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.