14 ms·
How we made compiler warnings fatal in Firefox
- kozak 9y agoWe 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 9y agoCan'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 9y agokozak 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 9y agoassignments inside a conditional check I see this in C code often. Why do people seem to like doing this?
- 77pt77 9y agoMasochism?
- noselasd 9y agoBecause while ((c = getchar()) != EOF) { // } Is considered more idiomatic and readable than any alternatives that's not assigning to c within the conditional. You'll see similar idioms in many other languages, e.g. while((line = reader.readLine()) != null) { } But if you're talking about e.g. if (foo = (bar & 0xf)) { } It's just bad style ...
- prewett 9y agoI think it's more idiomatic because otherwise you have to do: char c = EOF; while (c != EOF) { c = getchar(); ... } The idiomatic way reads better: "while next character is not EOF". Your (excellent) example of bad style is bad because it reads poorly: "if the result of the assignment of bar & 0xf to foo is true". Not only that, but it is not obvious what the result of an assignment is, if anything. (Yes, I do know it is the value, but personally, I think <nothing> is a more logical result).
- masklinn 9y agoDon't you mean char c = getchar(); while (c != EOF) { c = getchar(); … } ? Which not only reads badly but requires writing the "producer" twice. Though alternatively you could: while (1) { c = getchar(); if (c == EOF) { break; } ... }
- tbirdz 9y agochar c; do { c = getchar(); } while (c != EOF);
- noselasd 9y agoThis is worse, in most cases as you normally want to do something to c after you have read it, forcing you to re-check the condition int c; //a char can't represent EOF do { c = getchar(); if (c != EOF) { //do stuff } } while (c != EOF);
- mixedbit 9y agoI 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 9y agoFirefox 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 9y agoI've never seen any Gtk application that does not spew messages like this at least occasionally.
- yrro 9y agoDevelopers should be running with G_DEBUG=fatal-criticals so that those warnings convert into program crashes. But then again developers shouldn't have been writing anything in a language that requires manual resource management since C++.
- Gikoskos 9y agoI'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.
- ordu 9y agoWinAPI does not print into console warnings about accidental attempt to drop ref-counted object that has dropped already. So how can you know? Maybe it happens with WinAPI all the time silently.
- 9y ago
- bla2 9y agoI wonder how much harder this approach will make compiler updates for them.
- InclinedPlane 9y agoI used to do very frequent compiler updates for... a rather large codebase, it's not too bad. Even then it's only a part time job for one person, for a more reasonable code base (like firefox) and a more reasonable update schedule (official compiler releases) it's even easier. Basically you just snap off a dev branch from some stable build, do the update and identify all the failures then go through each one. Take a little bit of time to identify the problems and fix them as you go. If something looks like it's beyond your expertise level or requires the special expertise of the "code owners" to fix properly don't bother trying to wrap everything up in a nice tidy package all at once simply use pragmas or what-have-you to disable the warnings for the smallest section of code possible and file bugs (at a relevant level of priority) to fix up the issue properly later. Every once in a while you'll run into an annoying hairball kind of problem that requires a fair amount of work to fixup (like, say, warnings in generated code or something) but most of the time it's fairly trivial stuff.
- Aissen 9y agoThey specifically chose warnings one by one(e.g -Wunused-but-set-variable) instead of by groups (-Wall) to avoid that being an issue.
- larschdk 9y agoThat 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 9y agoI 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?
- hathym 9y ago#pragma warning( disable : xxxxx )
- kazinator 9y ago> 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!
- paol 9y agoRead the article. They choose which warnings are considered fatal.
- gommm 9y agoThey do say this at the start: > Choose with some care which warnings end up fatal. Don’t be afraid to modify your choices as time goes on. So, my understanding is that they didn't make all warnings fatal. Only the ones they believe are important.
- user5994461 9y agoI 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.
- crispweed 9y agoOne 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 9y agoThat's always something where I think current build systems are failing.
- softawre 9y agoBut if you didn't change a file, do you really want to see warnings for it? Wouldn't you rather just see warnings for stuff you changed and see all of the warnings when a full build is done in CI?
- crispweed 9y agoI guess the ideal would be the possibility to see both.
- connorcpu 9y agoPerhaps 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
- 9y ago
- josteink 9y ago> 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)
- nnethercote 9y agoTo be fair, the process of making them fatal was a lot more recent. Probably only took 3 or 4 years!
- daemin 9y agoIt also means that this is mainly applicable on software that is expected to be in development for many years and then have many years of active maintenance.
- bluGill 9y agoSoftware that is not expected to be in development for years probably doesn't need this. Get it working, manually tested, ship it and move on. This is probably cheaper than the extra engineering effort to make everything perfect. Most warnings are for things that won't matter since you won't port to that system (it doesn't matter if your invoke undefined behavior if the only compiler you use at your optimization level does what you want) All of the above will come back to bite you hard if you have to release again. Some will come back to bite you right away (the expense of manually testing everything), while others can lurk hidden for years (little vs big endian issues).
- lomnakkus 9y ago> (it doesn't matter if your invoke undefined behavior if the only compiler you use at your optimization level does what you want) I mean, in one sense you're right, but UB can make even trivial maintenance absolute hell even if you stick to the exact same build toolchain.
- dredmorbius 9y agoThe 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."
- Ono-Sendai 9y agoHow is ALLOW_COMPILER_WARNINGS implemented?
- nnethercote 9y agoFirefox has a custom build system, and it's implemented within that. So it's not something that will translate to another project, alas.
- ansgri 9y agoHow 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 9y agoI'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
- ansgri 9y agoThanks, this looks like a better solution!
- wfunction 9y agoHow 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 9y agoThe 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.
- jcoffland 9y agoThat's good to know. I usually just add a warning suppression or two.
- valuearb 9y agoThe 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 9y agoIf 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 9y agoYea, 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 9y agoThanks.
- 9y ago
- ryandrake 9y agoI 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!"
- Pxtl 9y agoHonestly, if there's a warning you've accepted as a known issue or inapplicable to your code, just suppress it. Maybe document it as a low priority bug in the tracker so it has a place on the to-do list, but get it out of the compiler results.
- munificent 9y agoRelated: "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 9y agoAlso we will never actually do that last part.
- frozenport 9y agoRelated: "We're not sure if this product has a market, and the company that will acquire our tech will integrate it with their stack."
- jcoffland 9y agoI'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 9y agoDid 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 9y agoIt 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 9y agoThat’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-guide-and-reference-pragmas https://software.intel.com/en-us/cpp-compiler-18.0-developer...), 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#Diagnostic-Pragmas https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#D..., https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas https://clang.llvm.org/docs/UsersManual.html#controlling-dia...): #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)
- convefefe 9y agoHave 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.
- TD-Linux 9y agoI think that's why TFA suggested only doing this in CI (with a known compiler version) and not for local builds.
- nnethercote 9y agoFrom 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."
- cratermoon 9y agoSince 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.
- thephyber 9y agoRust laughs and pats Go on the head.
- cratermoon 9y agoYou seem nice.
- mort96 9y agoI 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 9y agoWhy? What's useful about unused variables or unreachable code?
- mort96 9y agoIt's not useful in shipped code, and absolutely should be a warning, but it's incredibly useful while creating the program or debugging.
- Twirrim 9y agoGo isn't even remotely close to strict. They optimised for speed vs correctness checking.