Request: Highest warning level and warning is error

[mreuvers]

Hi there,

When I upgraded our 2.67 trunk to the latest code, I was introduced with a buckload of new compile warnings, ranging from unused variables, implicit casts, unreachable code to hided virtual functions.

Would it be possible to enforce a much stricter warning policy on the code? That is: the highest warning level and all warnings need to be fixed (warning = error). Personally I think that enforcing such a policy will increase the portability of the code, but more importantly: it will guide as an early indicator of bad code (read: bugs). It'll also aid in the design of your code. For instance on a lot of occasions you are casting back and forth between ints and unsigned ints. In that case, I'd say make up your mind, is it an int or an unsigned int? Getting a warning about that, forces you to reconsider the design.

Even better would be to run PC lint (http://www.gimpel.com/html/pcl.htm) on the code. But... I'm afraid that that will produce A LOT of warnings It won't hurt to address them though at one point in time.

Cheers,


Martijn

[chunky]

I used to feel the same way [-Wall -Werror], right up until the moment I switched compiler version: all my old code started throwing warnings where it hadn't before, which caused compilation errors, and I started getting emails from random people saying my code was busted.

I do concur that code should be as warningless as possible, and people in the past have supplied patches on this very forum to fix all the warnings [hint hint]. But -Wall -Werror has bitten me personally in the past, and I don't think it's a good idea to use it in any project that you release the source for and expect people to compile it themselves.

Gary (-;

[sparkprime]

I personally feel -Werror is never useful. As a developer I want information from my tools. I don't want my tools to disobey my instructions (i.e. fail to create an executable) unless the problems are so severe that they are forced to give up.

However I always compile my own projects with the strictess warning levels and have found this to make porting between compilers much easier. My only worries are msvc's stupid "security" warnings which can be turned off, and the fact that different versions of gcc produce different warnings.

I get quite irritated when I can't include 3rd party headers in my project without picking up lots of warnings from them, but gcc's -isystem can help deal with this (unless there is a syntax error in the header, which has happened before with bullet).

I could potentially help eliminate warnings from bullet but I think there are bigger priorities at the moment.

[mreuvers]

all my old code started throwing warnings where it hadn't before, which caused compilation errors, and I started getting emails from random people saying my code was busted.

Compiler switching doesn't occur very often, and if it does you indeed need to patch your code, if necessary, before committing. A warning is not the compiler trying to annoy you, it's an indication that something that is wrong with your code. It might be an indicator of a bug. If another compiler informs you with warnings, I would choose to fix these warnings, rather than disabling them. Internally our buildserver for instance, makes sure that all code that we produce compiles for the PC, Wii and DS... warning free. If some code produces a warning (=error), the responsible programmer immediately receives an e-mail asking him to address the issue. Having multiple compilers (and possibly PC Lint) check your code for warnings, definitely solved a lot of potential bugs for us.

The more complex a project becomes, the more urgent it becomes to define a good warning/error policy.

But -Wall -Werror has bitten me personally in the past, and I don't think it's a good idea to use it in any project that you release the source for and expect people to compile it themselves.

I don't see why not. Of course Bullet needs to indicate what compilers it supports, so people won't complain that the code breaks on compiler X.

I personally feel -Werror is never useful. As a developer I want information from my tools. I don't want my tools to disobey my instructions (i.e. fail to create an executable) unless the problems are so severe that they are forced to give up.

Unfortunately I strongly disagree. IMO -Werror is *always* useful. The information you get from the warnings is an indication that something is (seriously) wrong with your code. If you have unused variables, unreachable code, hided virtual functions (all part of the current Bullet trunk), you can of course choose to ignore them and ship the code. However a much better practice is that people fix these issues first.

Unfortunately when not enforcing warning = error, people are not forced to do so causing warnings to slip into the project again. As a matter of fact, I completely got rid of all warnings produced by MSVC9.0 (and the Wii compiler) in Bullet 2.67. When I upgraded to 2.72 a buckload of new warnings where introduced. It took me a couple of hours again to make the latest Bullet version warning free again, however IMO it would be better if some policy is introduced here that forced people to commit warning free code. And be honest; what's wrong with warning free code? It won't hurt anyone, in fact, it will help you to create better code!

Of course we can have a debate on exactly what warnings are useful and which ones aren't. But the useful warnings that we can agree upon, should definitely be enforced IMO.

Just my two cents

[sparkprime]

I agree that warning-free code is the best policy, except for three things:

- a lot of warnings are to do with style or whatever, not correctness

- warnings require good program analysis in the compiler, and this is inherently unstable. If you simplify or complicate the code a bit, warnings can appear and disappear as the results of the program analysis change. Sometimes I have gotten bogus warnings that are not possible to "fix" without losing performance, so I leave them there. This is rare but when you enforce a global policy it becomes a problem.

- development means testing incomplete code, and incomplete code will have warnings. The average dev work doesn't want/need -Werror, it gives you no more information and gets in the way if you comment out code that uses a parameter or whatever.

On the other hand, users building bullet will want -Werror since then errors with their system (like missing headers) will manifest as single errors rather than 3 pages of errors/warnings.

[mreuvers]

- a lot of warnings are to do with style or whatever, not correctness

I've to disagree again Warnings are all about correctness, not style. The style of how you write C++ code really doesn't matter to a compiler, the preprocessor simply removes all useless whitespace before it passes the text onto the compiler anyhow. It's the construction/actual implementation of your code that a compiler might dislike, and usually for a good reason! IMO it's not a 'style' to ignore implicit casts, it's usually the result of bad design or evolutionary programming.

- warnings require good program analysis in the compiler, and this is inherently unstable. If you simplify or complicate the code a bit, warnings can appear and disappear as the results of the program analysis change. Sometimes I have gotten bogus warnings that are not possible to "fix" without losing performance, so I leave them there. This is rare but when you enforce a global policy it becomes a problem.

Can you give me an example of these appearing bogus bugs? Apart from some meta programming in macros maybe, which is bad anyhow, I never ran into such a bug. Also the 'unstable' program analysis is not something we ran into with any of the compilers we use. Can you give me an example of that too?

- development means testing incomplete code, and incomplete code will have warnings. The average dev work doesn't want/need -Werror, it gives you no more information and gets in the way if you comment out code that uses a parameter or whatever.

In my opinion library code of any library should be warning free. It's up to the end-user if he follows that strictness or not. However IMO the people that actually contribute to the library itself, should make sure that the code they commit is warning free. The larger a project grows, the more important this issue becomes. If no strict coding guidelines and warning policies are enforced, eventually the project can become very unstable and nigh on impossible to maintain, let alone to change or refactor.

[chunky]

I still maintain that it's impossible to *guarantee* warning-free-ness. What happens when the new GCC comes out with a warning that it didn't previously have? The Bullet version released the day before [with -Wall -Werror] will now refuse to compile despite everyone's best efforts and good faith that it will.

An example of stylistic warnings is -Weffc++, which turns on warnings for things that don't meet the recommendations in Scott Myers' books.

Understand that I don't think anyone is opposed to fixing warnings, we all know that warnings are bad and that the idealist doesn't have any. But I think it's a bad idea for end users to have code that refuses to compile just because they're using a different compiler to me.

Gary (-;

[Erwin Coumans]

Here are some thoughts:

• We need to make sure that releases are as warning-free as possible, at least on the platforms Bullet developers use: Win32 MSVC 2005/2008, OSX Leopard, Linux gcc 4.x.
• We are not going to add -Werror to any of the build systems for now
• There are too many different compiler versions (and newer compilers than our Bullet release) to get rid of each warning
• We don't want to reject useful patches because formatting or warnings
• If your compiler has warnings in trunk or official release, please submit a new patch to the issue tracker. Don't mix such patch with any other changes or new functionality.
• One way to reduce warnings for upcoming Bullet releases is to create 'make dist' to create a release distribution, and add Werror for this special 'make dist' build.
• We should create automated unit tests, with proper feedback
• We should use Valgrind and similar tools to improve quality. Usage of 'glut' might prevent proper Valgrind use, so should create non-graphical command-line tests. Any volunteers to provide Valgrind reports?

Thanks,
Erwin

by the way, chunky and sparkprime and others: the library name Bullet is with capital B

[chunky]

I'm pretty familiar with valgrind, I'll gladly help out.

I'm not particularly experienced with writing unit tests, but I have access to varying machines of varying architectures, with valgrind on them. Just as an aside, I think pretty useful would be if the unittests occupied a whole top-level directory of their own.

Gary (-;

PS One day I'll get my head around Bullet always having a capital B at the start.

[sparkprime]

I fixed some errors here that were preventing me from turning on warnings when compiling bullet.

http://code.google.com/p/bullet/issues/detail?id=123

I also fixed a couple of warnings in headers that were cluttering the compiler output from my own project.

Note: there's no reason to omit -g when compiling an optimised Bullet. There is no performance loss. Binaries can always be stripped later to reduce executable size.

Note2: there is a lot of use of long long in the Collada stuff, so to compile bullet with -ansi -pedantic -Wall -Wextra it is necessary to also specify -Wno-long-long to suppress errors about long long not being valid c++.