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

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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;
       ^
}

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Arnaud Sourioux [:Six]]

Attachment: remove_warning_private_field_mEnabled_unused.patch

This patch is based on the discussion we add with Dholbert and the explanation he wrote in his last comment.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Arnaud Sourioux [:Six]]

Attachment: remove_warning_private_field_mEnabled_unused.patch

This version is seeting mEnabled as false.

Comment 5

[Arnaud Sourioux [:Six]]

Tbpl looks green:
https://tbpl.mozilla.org/?tree=Try&rev=6c55d25513c3

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8481135 [details] [diff] [review]
remove_warning_private_field_mEnabled_unused.patch

Ted: adding you as an alternate reviewer (since you reviewed bug 1006051), in case you can get to this before Doug does.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8481135 [details] [diff] [review]
remove_warning_private_field_mEnabled_unused.patch

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

I only reviewed build bits in that patch, I'm not a peer of this code.

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Arnaud Sourioux [:Six]]

Attachment: suppress_warning_private_field_mEnabled_unused.patch

This is a new version including dholbert's feedbacks (Thanks btw)

Comment 10

[Daniel Holbert [:dholbert]]

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)

Comment 11

[Daniel Holbert [:dholbert]]

Smaug said in IRC that he'd be up for r+'ing this.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks, jaws!

Comment 13

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a12875dfd1c

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5a12875dfd1c