Closed
Bug 1232219
Opened 7 years ago
Closed 7 years ago
Enable -Wunused for C code
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(5 files, 2 obsolete files)
6.40 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
7.73 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
4.03 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.20 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
4.90 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 2•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 3•7 years ago
|
||
Attachment #8698276 -
Flags: review?(netzen)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
Attachment #8698277 -
Flags: review?(karlt)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8698279 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
Attachment #8698296 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Comment 7•7 years ago
|
||
> 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.
Updated•7 years ago
|
Attachment #8698276 -
Flags: review?(netzen) → review+
![]() |
Assignee | |
Comment 8•7 years ago
|
||
(Need to allow compiler warnings in libtremor/ for Android.)
Attachment #8698313 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8698279 -
Attachment is obsolete: true
Attachment #8698279 -
Flags: review?(mh+mozilla)
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 11•7 years ago
|
||
Attachment #8699358 -
Flags: review?(hsivonen)
![]() |
Assignee | |
Comment 12•7 years ago
|
||
Updated to not ignore warnings in parser/expat/lib/.
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8698313 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 13•7 years ago
|
||
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+
Attachment #8699358 -
Flags: review?(hsivonen) → review+
Comment 14•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 15•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 16•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 17•7 years ago
|
||
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•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•4 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•