1232219 - Enable -Wunused for C code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [:njn]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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.

Comment 2

[Nicholas Nethercote [:njn]]

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

Comment 3

[Nicholas Nethercote [:njn]]

Attachment: (part 1) - Fix -Wunused warnings in libmar/

Comment 4

[Nicholas Nethercote [:njn]]

Attachment: (part 2) - Fix -Wunused warnings in widget/gtk/

Comment 5

[Nicholas Nethercote [:njn]]

Attachment: (part 3) - Fix remaining -Wunused warnings

Comment 6

[Nicholas Nethercote [:njn]]

Attachment: (part 4) - Enable -Wunused for C code, except where it's too annoying

Comment 7

[Nicholas Nethercote [:njn]]

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

Comment 8

[Nicholas Nethercote [:njn]]

Attachment: (part 3) - Fix remaining -Wunused warnings

(Need to allow compiler warnings in libtremor/ for Android.)

Comment 9

[Mike Hommey [:glandium]]

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?

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Nicholas Nethercote [:njn]]

Attachment: (part 2.5) - Fix -Wunused warnings in parser/expat/lib/

Comment 12

[Nicholas Nethercote [:njn]]

Attachment: (part 3) - Fix remaining -Wunused warnings

Updated to not ignore warnings in parser/expat/lib/.

Comment 13

[Nicholas Nethercote [:njn]]

Comment on attachment 8699359 [details] [diff] [review]
(part 3) - Fix remaining -Wunused warnings

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

Carrying over r+.

Comment 14

[Karl Tomlinson (:karlt)]

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.

Comment 15

[Nicholas Nethercote [:njn]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a785337924021ba18a5252775472a2ead8a50f6
Bug 1232219 (part 1) - Fix -Wunused warnings in libmar/. r=bbondy.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8acbf93a07173240edb5818cf4361036d8f85122
Bug 1232219 (part 2) - Fix -Wunused warnings in widget/gtk/. r=karlt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7950a7c0fcdb8324a8bd4e2abac305f58aaa32c5
Bug 1232219 (part 2.5) - Fix -Wunused warnings in parser/expat/lib/. r=hsivonen.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7179b0403756813c7779451660d94290821cf3
Bug 1232219 (part 3) - Fix remaining -Wunused warnings. r=glandium.

Comment 16

[Nicholas Nethercote [:njn]]

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.

Comment 17

[Nicholas Nethercote [:njn]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26e9a666179bf822f030ed8322b3a032ffa4b496
Bug 1232219 (follow-up) - Fix nsinstall.c bustage in SM(e) builds. r=me.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a7853379240
https://hg.mozilla.org/mozilla-central/rev/8acbf93a0717
https://hg.mozilla.org/mozilla-central/rev/7950a7c0fcdb
https://hg.mozilla.org/mozilla-central/rev/ca7179b04037
https://hg.mozilla.org/mozilla-central/rev/540f77351ef1
https://hg.mozilla.org/mozilla-central/rev/26e9a666179b