
How we made compiler warnings fatal in Firefox - nachtigall
https://blog.mozilla.org/nnethercote/2017/07/05/how-we-made-compiler-warnings-fatal-in-firefox/
======
ryandrake
I remember my first programming job straight out of school, where a full clean
build of our project emitted 10K+ warnings, and I thought to myself, "Is this
really how professional programming is?" Asked a co-worker why on Earth we
don't fix them, as there may be serious bugs hiding in there, and the response
was "We're too busy fixing bugs!"

~~~
munificent
Related:

"We're going to rush through implementation so that we have lots of time left
at the end to fix all the bugs caused by rushing through implementation."

~~~
draw_down
Also we will never actually do that last part.

~~~
hinkley
But I get to look high-minded by participating in the hand-wringing about the
problem without actually moving a muscle to improve the situation.

~~~
draw_down
The last refuge of the scoundrel!

------
josteink
> Congratulations! You now have fatal warnings on everywhere that is
> practical.

I think that last bit is the most important one. You need to be practical
about stuff like this.

Putting a ban-hammer down like a CI-nazi is most likely not going to get you
the results you were hoping for.

Edit: Looking at the linked issue [1], this has taken _quite_ some time. First
comment on that issue is 15 years old :D

[1]
[https://bugzilla.mozilla.org/show_bug.cgi?id=187528](https://bugzilla.mozilla.org/show_bug.cgi?id=187528))

~~~
filereaper
I think that linked bugzilla issue is amazing, slow patient work over years to
fix issues. We should really celebrate this sort of effort more in our
industry.

~~~
sliverstorm
Move slow and fix things?

~~~
still_grokking
Pun intended? ;-)

------
crispweed
One big reason to make compiler warnings fatal (not mentioned here, as far as
I can see) is the fact that otherwise, unless you do a full rebuild, you won't
see warnings for any source files that don't need to be rebuilt.

~~~
jhasse
That's always something where I think current build systems are failing.

~~~
connorcpu
Perhaps not build systems, but I think editors that help you keep track of
warnings should probably have a list of them per-file and only clear that list
when the file actually gets rebuilt

~~~
jhasse
I don't know, I'd rather have that functionality in the build system. The
editor should just parse the build system's output and keep the stuff it does
to a minimum.

------
ansgri
How do you deal with thirdparty includes, particularly with big header-only
libraries? You basically have to change compiler flags on the fly, and AFAIK
it isn't well supported. My colleagues have dealt with this by defining some
ugly macros which enable/disable hardcoded warnings, and there are lots of
them.

~~~
humanrebar
I've had good success including not-my-code with the appropriate compiler
flag. This tells the compiler that I'm not interested in warnings in those
files since I'm not going to change them.

[https://gcc.gnu.org/onlinedocs/cpp/System-
Headers.html](https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html)

~~~
wfunction
How does this work with templates? If a type you pass into such a library
causes a warning in that library, is that your fault or the library's? How
could the compiler possibly tell? It seems like this can't even work in
principle if there are templates involved.

~~~
humanrebar
The warnings I've had issues with were warnings regardless of whether the code
was templates: ignored return values, unused parameters, etc.

There may be cases where the potential false negatives are unacceptable. Be
judicious, but in my case the benefits far outweighed the risks.

Of course, it's a great idea to provide upstream patches and issues whenever
possible.

------
dredmorbius
The hardest part about where you are and where you want to be is the getting
there from here.

This is some good practical advice (and a welcome improvement) from Mozilla.

I'm also reminded of an email I sent out to a technical mailing list some time
back, in response to a user's question about suppressing warning messages when
running his program (this for a runtime-based interpreter).

Various suggestions came for how to limit, suppress, or remove warnings from
the log.

My response, which apparently earned some favourable consideration at the
vendor:

"Fix the bugs in your code."

------
jcoffland
I'm surprised it took them this long. I've used ``-Wall -Werror`` on all my
code for decades. If you use it from the start it's easy to maintain and
catches a lot of potential bugs. There are the occasional false positives and
warnings coming from 3rd party headers. For these I add warning specific
suppressions.

~~~
Someone
Did you build on many different platforms using multiple compilers with
various degrees of standards support, just as Mozilla does? That makes it
harder (for starters: how do you suppress warnings in a portable way?)

~~~
frozenport
It would be nifty to supress warnings on a per include directory level by
passing flags directly to the compiler. This is how static analysis tools like
PVS avoid flooding us with errors.

~~~
Someone
That’s what Mozilla does, too, reading the article.

However, that’s not what the grandparent wrote: _" For these I add warning
specific suppressions”_. I read that as disabling single warnings, not a
compilation unit at a time.

For example, in Visual Studio, you can do ([https://msdn.microsoft.com/en-
us/library/2c8f766e.aspx](https://msdn.microsoft.com/en-
us/library/2c8f766e.aspx)):

    
    
       #pragma warning(push)
       #pragma warning(disable: 1234)
       ...
       #pragma warning(pop)
    

Intel’s compiler is compatible with this ([https://software.intel.com/en-
us/cpp-compiler-18.0-developer...](https://software.intel.com/en-us/cpp-
compiler-18.0-developer-guide-and-reference-pragmas)), but it would surprise
me if it used the same warning numbers.

GCC and clang, on the other hand, need
([https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-
Pragmas.html#D...](https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-
Pragmas.html#Diagnostic-Pragmas),
[https://clang.llvm.org/docs/UsersManual.html#controlling-
dia...](https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-
via-pragmas)):

    
    
       #pragma GCC diagnostic push
       #pragma GCC diagnostic ignored “-W...”
       ...
       #pragma GCC diagnostic pop
    

Because you ideally only want to target the exact place that triggers a
warning, either is fairly wordy, and it gets worse if you want to cater for
all three compilers.

It also wouldn’t surprise me if Mozilla supported multiple versions of gcc
(for instance because they want to build for an OS with long-term support, or
for an obscure OS for which the latest gcc is not (yet) available).

(_If_ you use C99, at least with gcc and clang, you can simplify that by using
__Pragma_ , which has the big advantage that you can use it in preprocessor
macros, so that you may be able to simplify what things look like in the
source)

~~~
frozenport
>> ideally only want to target the exact place that triggers a warning

On the other hand, its typically some library that is messing up the static
analysis tool.

While the #pragama options are good for disabling warnings in typical
projects, from personal experience they are broken in NVCC.

------
kozak
We did the same for the linter in our JavaScript project. Of course, the
linter config is focused mostly on pitfalls and "good parts", not on code
style (except for the most annoying code style issues).

~~~
vanderZwan
Can't most of these code-style issues be auto-formatted on save these days?

Years ago when I still programmed in Go I made use of that all the time: if it
didn't format on save I knew I had a compiler error somewhere.

~~~
masklinn
kozak is specifically saying their linter is mostly focused on any _non-
codestyle_ issue.

And compiler errors are not worth linting given you can just do a syntax
check, linting is useful to avoid or require disambiguation of error-prone
constructs.

Examples in JS would be `with`, `==`, assignments inside a conditional check,
parseInt without a base (mostly in older browser), parseFloat, non-prefixed
octal literals (073 rather than the explicit hex-style 0o73), string
expression statements (outside of the "use strict" pseudo-pragma), and several
scoping errors (which strict mode will mostly only check at runtime). All of
these are syntactically perfectly valid, but more often than not they're also
mistakes or sources of errors.

~~~
nerdponx
_assignments inside a conditional check_

I see this in C code often. Why do people seem to like doing this?

~~~
masklinn
As demonstrated by other commenters, makes for a terse iterator-ish pattern in
languages without iterators (like C, or until recently Javascript) e.g.

    
    
        while ((c = getchar()) != EOF) {
         //
        }
    

in a language with iterators would be

    
    
        for (c in chars()) {
         //
        }

~~~
masklinn
And note that this C pattern only works when you can pass the "end of
iteration" signal in-band, if you can't you will need to use more complex
pseudo-iteration protocols e.g.

    
    
        while(generator(&c)) {
            //
        }

------
mixedbit
I remember compiling and working with Firefox code many years ago, one thing
that surprised me were non-fatal assertion errors continuously printing errors
to console in debug build. It looked like Netscape legacy, I wonder if they
managed to clean this.

~~~
avian
Firefox ESR 52.2.0 on Debian still prints a whole bunch of assertion errors if
you run it in a terminal:

    
    
        (firefox-esr:345): GLib-GObject-CRITICAL **: g_object_ref: assertion 'object->ref_count > 0' failed

~~~
dom0
I've never seen any Gtk application that does not spew messages like this at
least occasionally.

~~~
Gikoskos
I've noticed this as well in all the Gtk apps that I've run through the
terminal.

Never having programmed in Gtk, what is the cause of this? Is Gtk broken or
are all the programmers using it incorrectly? I've programmed a great deal in
WinAPI and stuff like that never happens.

~~~
onli
No one knows how to use GTK correctly, not even the GTK developers. At least
that was the situation in 2014 when subsurface moved from GTK to Qt,
[https://www.youtube.com/watch?v=ON0A1dsQOV0](https://www.youtube.com/watch?v=ON0A1dsQOV0).

~~~
77pt77
I think this is very common.

There's a bunch of boilerplate code that just gets carried from project to
project.

My favourite was when KDE decided to move to cmake. People realized that most
m4/autoconf macros were there for no reason and no one actually knew what they
checked for let alone how.

~~~
revelation
You might like this essay:

[http://queue.acm.org/detail.cfm?id=2349257](http://queue.acm.org/detail.cfm?id=2349257)

~~~
77pt77
> Sam Leffler's graphics/libtiff is one of the 122 packages on the road to
> www/firefox, yet the resulting Firefox browser does not render TIFF images

Don't even get me started on the circular dependencies that need to be broken
by installing packages with some options disabled. Kind of an awkward
bootstrap.

~~~
rleigh
I added CMake support to libtiff last year. Haven't checked if the ports tree
uses it yet.

------
convefefe
Have done something similar, and it works, but ...

... the downside is that every time someone uses a different version of your
normal compiler, or a different compiler, you have an unfortunately large
chance of having the build fail because a new bit of code that previously was
warning free is now determined to be warning-worthy.

Not a huge problem (and better than cultivating the habit of ignoring
warnings), but it is a time cost that has to be paid now and again for
committing to halting the build on a static analysis failure.

~~~
nnethercote
From the blog post:

"Set things up so that fatal warnings are off by default, but enabled on
continuous integration (CI). This means the primary coverage is via CI. You
don’t want fatal warnings on by default because it causes problems for
developers who use non-standard compilers (e.g. pre-release versions with new
warning classes). Developers using the same compilers as CI can turn it on
locally if they want without problem."

------
valuearb
The problem with my work project is that most of the warnings are from third
party libraries (Cocoapods since it's an iOS project). Most of them from
Jainrain, which has publicly said they don't care.

~~~
MaxGabriel
If you're copy-and-pasting source code into your project, you can ignore all
warnings on a per-file basis (See
[https://stackoverflow.com/a/6921972/1176156](https://stackoverflow.com/a/6921972/1176156)).

And if you're using a package manager like Cocoapods, it shouldn't be a
problem.

Edit: Wait I just re-read your post again and you are using Cocoapods. Can you
not just turn on "treat warnings as errors" to YES for your main project, but
leave it off for your main project? Or just add `inhibit_all_warnings!` to the
top of your Podfile, like on this example file:
[https://guides.cocoapods.org/syntax/podfile.html](https://guides.cocoapods.org/syntax/podfile.html)

~~~
ryandrake
Yea, I've had success with the following procedure:

1\. Determine the set of warnings you're going to enable for you project, and
apply them to your own code and all third-party libraries.

2\. For each third party dependency, find the minimal set of warnings that,
when suppressed, make them compile cleanly, and disable just those (only for
that specific third party project).

3\. If any of those suppressions makes you uncomfortable, submit a patch to
the third party maintainer if possible :)

As for #3 with open source libraries, I've seen the spectrum of attitudes from
"thank you very much for the patch!" to "we don't care about compiler
warnings, just turn them off"

EDIT: One thing this doesn't address is third party dependencies where
#including their headers from your project results in warnings. If you're a
developer who writes header files that produce warnings, please take a moment
to hang your head in shame.

~~~
valuearb
Thanks.

------
bla2
I wonder how much harder this approach will make compiler updates for them.

~~~
Aissen
They specifically chose warnings one by one(e.g -Wunused-but-set-variable)
instead of by groups (-Wall) to avoid that being an issue.

~~~
larschdk
That helps, but is not bullet proof. Changes to optimization passes can cause
warnings to be emitted in cases where they weren't previously. For example,
detection of an unused result of an expression.

~~~
viraptor
I can't really think of any case where this is possible. Warnings are produced
(only?) before the serious optimisation passes. Have you got an example where
unused result warning changes between optimisation levels?

~~~
pm215
These are, or used to be, quite common with gcc. Unused-result warnings
require dataflow analysis to run to determine whether the values are used, and
on no-optimisation compiles gcc didn't bother to do dataflow analysis.

Conversely, here's one where a warning is produced only without -O2:

    
    
      orth$ cat z.c
      int foo (int a) {
         int x;
         if (a == 5) { x = 3; return 42; }
         return x;
      }
      orth$ gcc -O2 -Wall -o z.o -c z.c
      orth$ gcc  -Wall -o z.o -c z.c
      z.c: In function ‘foo’:
      z.c:4:4: warning: ‘x’ may be used uninitialized in this function [-Wmaybe-uninitialized]
          return x;
          ^
      orth$ gcc --version
      gcc (Debian 4.9.2-10) 4.9.2

------
cratermoon
Since I've been programming in the Go language for a few months now I've
really often compared the compiler's strictness with the C/C++ compiler's
default laxity. The Go compiler won't even let you leave in an unused import,
and there are a number of other things that in C would maybe generate a
warning that won't compile in Go.

~~~
mort96
I absolutely hate that you can't have unused variables in Go, or that you
can't have unreachable code in Go or Java. It makes the already tiresome work
of debugging significantly more frustrating.

~~~
cratermoon
Why? What's useful about unused variables or unreachable code?

~~~
mort96
It's not useful in shipped code, and absolutely should be a warning, but it's
incredibly useful while creating the program or debugging.

------
Ono-Sendai
How is ALLOW_COMPILER_WARNINGS implemented?

~~~
nnethercote
Firefox has a custom build system, and it's implemented within that. So it's
not something that will translate to another project, alas.

------
hathym
#pragma warning( disable : xxxxx )

------
kazinator
> _Also, before upgrading the compilers on CI you need to fix any new
> warnings. But this isn’t a bad thing._

It's, in fact, downright stupid and means you're writing in a different
programming language with each compiler upgrade and at the mercy of some
poorly-considered third-party opinions.

Many compiler warnings are just stylistic. They can even contradict each
other. Compiler A says, "suggest parentheses here". Oops, compiler B doesn't
like it: "superfluous parentheses in line 42".

As a rule of thumb, warnings that aren't finding bugs aren't worth a damn.

The proper engineering approach is to evaluate newly available warnings and
try them. Keep any that are uncovering real problems, or could prevent future
ones; enable those and not any others.

Warnings are just tools. You pick the tools and control them, not the other
way around.

Every code change carries risk!

~~~
user5994461
I don't know what languages you are thinking but firefox is mostly C and C++.

There are no superfluous parenthesis warnings in GCC and MSVC.

~~~
zoodlep
Uhh, perhaps I'm not parsing your claim correctly, but that warning most
certainly does exist in gcc.

    
    
      $ gcc -Wall test.c
      test.c: In function ‘main’:
      test.c:6:5: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
          if (a = b) {
          ^
    

Parentheses here would be superfluous, but would perhaps confirm you really
intended to assign (i.e. used = instead of ==).

~~~
user5994461
You realize that you have an assignment instead of a comparison?

The warning here couldn't be more justified. In a decent language, this should
be a hard error.

