Closed Bug 1010103 Opened 6 years ago Closed 5 years ago

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

Categories

(Toolkit :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dholbert, Assigned: Six)

References

(Blocks 1 open bug)

Details

Attachments

(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
Status: NEW → ASSIGNED
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:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsParentalControlsServiceAndroid.cpp#13

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]
remove_warning_private_field_mEnabled_unused.patch

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()
>+nsParentalControlsService::nsParentalControlsService() :
>+mEnabled(false)
> {
>+  mozilla::unused << mEnabled;

The "mEnabled(false)" there needs to be indented by 2 spaces. It should look like the other nsParentalControlsService implementations:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsParentalControlsServiceWin.cpp#28
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsParentalControlsServiceWin.cpp#28
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/parentalcontrols/nsParentalControlsServiceCocoa.mm#15
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)
Thanks!

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)
https://hg.mozilla.org/mozilla-central/rev/5a12875dfd1c
Status: ASSIGNED → RESOLVED
Closed: 5 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.