Enable -Wunused for C code

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla46

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

We set -Wno-unused for C code. The comment in configure.in says this is due to "lots of violations in third-party code".

But we have good mechanisms for dealing with third-party code warnings now, such as ALLOW_COMPILER_WARNINGS. And we don't set -Wno-unused for C++ code, so it's clearly not that hard to handle it. And it's a useful warning.

So let's turn it on for C code.
Most of the C code in mozilla-central is from third-party libraries, where we ALLOW_COMPILER_WARNINGS. Reporting those -Wunused warnings will add more build noise, though it would allow us to remove a special case from configure.in and maybe encourage upstream fixes.
(In reply to Chris Peterson [:cpeterson] from comment #1)
> Most of the C code in mozilla-central is from third-party libraries, where
> we ALLOW_COMPILER_WARNINGS. Reporting those -Wunused warnings will add more
> build noise, though it would allow us to remove a special case from
> configure.in and maybe encourage upstream fixes.

Hmm... it increases the number of -Wunused* warnings from 20 to 378 on my Linux box. That *is* a lot of build noise.

At the very least I'll post my patches that fix the things found by -Wunused in our code.
Attachment #8698279 - Flags: review?(mh+mozilla)
> Hmm... it increases the number of -Wunused* warnings from 20 to 378 on my
> Linux box. That *is* a lot of build noise.

With a couple of judicious uses of -Wno-unused* (see part 4) I got that back down to 41. That's a small enough increase over the status quo that I think it's ok.
Attachment #8698276 - Flags: review?(netzen) → review+
(Need to allow compiler warnings in libtremor/ for Android.)
Attachment #8698313 - Flags: review?(mh+mozilla)
Attachment #8698279 - Attachment is obsolete: true
Attachment #8698279 - Flags: review?(mh+mozilla)
Comment on attachment 8698296 [details] [diff] [review]
(part 4) - Enable -Wunused for C code, except where it's too annoying

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

::: memory/mozjemalloc/moz.build
@@ +44,5 @@
>  
>  if CONFIG['GNU_CC']:
> +    CFLAGS += [
> +        '-Wshadow',
> +        '-Wno-unused',      # too many annoying fatal warnings from MFBT

from mfbt?
Attachment #8698296 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8698313 [details] [diff] [review]
(part 3) - Fix remaining -Wunused warnings

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

::: tools/rewriting/ThirdPartyPaths.txt
@@ +55,5 @@
>  netwerk/sctp/src/
>  netwerk/srtp/src/
>  nsprpub/
>  other-licenses/
> +parser/expat/lib/

IIRC, our copy of expat is patched beyond any hope of pulling an upstream. I'm not sure whether that qualifies as third party code anymore. Maybe hsivonen would know.

r+ with that figured out.
Attachment #8698313 - Flags: review?(mh+mozilla) → review+
Updated to not ignore warnings in parser/expat/lib/.
Attachment #8698313 - Attachment is obsolete: true
Comment on attachment 8699359 [details] [diff] [review]
(part 3) - Fix remaining -Wunused warnings

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

Carrying over r+.
Attachment #8699359 - Flags: review+
Comment on attachment 8698277 [details] [diff] [review]
(part 2) - Fix -Wunused warnings in widget/gtk/

> 
>     /* Add style property to GtkEntry.
>      * Adding the style property to the normal GtkEntry class means that it
>      * will work without issues inside GtkComboBox and for Spinbuttons. */
>-    entry_class = g_type_class_ref(GTK_TYPE_ENTRY);
>+    g_type_class_ref(GTK_TYPE_ENTRY);

Please remove this line and comment altogether and the 
g_type_class_peek(GTK_TYPE_ENTRY) code in moz_gtk_shutdown().

The warning is telling us that entry_class is no longer used, so I don't want
to suppress the warning without removing the code that sets it.

>-    GtkRequisition requisition;
>+    /* GtkRequisition requisition; */

Please remove altogether.

The rest looks great, thanks.
Attachment #8698277 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/540f77351ef1eed4f32a585eb5003cf86ebb5482
Bug 1232219 (part 4) - Enable -Wunused for C code, except where it's too annoying. r=glandium.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.