5

our project manager recently set up a policy to request developers to remove ALL the compiler warnings. I understand some warnings really are easy to be removed, some are not so easier. So some developers uses every possible way to achieve the goal. For example, using explicit cast to cast double to float, float to int, signed to unsigned etc, etc. Since our code base is so large, over 20 years of 30-50 developers' work, I really doubt that how much this effort can really help us, if it does have some merits. Can anyone give some of your advice or arguments? our project uses C++.

Cœur
  • 37,241
  • 25
  • 195
  • 267
nowgains nowpains
  • 185
  • 2
  • 10
  • 8
    I guess that the warnings that are not easy to remove are the important ones. – R. Martinho Fernandes Jan 25 '13 at 15:42
  • This is a duplicate of [C/C++ compiler warnings: do you clean up all your code to remove them or leave them in?](http://stackoverflow.com/q/183788/62576). It's also not proper here, as it's a discussion question. Voting to close. – Ken White Jan 25 '13 at 15:43
  • 2
    "our project manager recently set up a policy" - he's probably a smartass. –  Jan 25 '13 at 15:43
  • 3
    It should be easy - `#pragma warning(disable:...)`... – Luchian Grigore Jan 25 '13 at 15:43
  • 5
    I have the policy that code should generate no hints or warnings as well. If you *properly* fix them (not by just blindly adding typecasts, etc.), it makes it easier to spot **real** problems in your code. Not fixing them properly is stupid, but it's also a waste of time. – Ken White Jan 25 '13 at 15:46
  • 4
    Personally I agree with your boss. I believe every warning should be regarded as an error. And if you really have a good reason not to rewrite some code that generates a warning (maybe it comes from a third-party library, maybe it is a compiler bug, maybe rewriting working legacy code would be just too error-prone), then you should make this explicit via a `#pragma` directive – Andy Prowl Jan 25 '13 at 15:46
  • Moreover, there are many dangerous situations for which some compilers do *not* generate any warning (e.g. forgetting to initialize a member variable in a constructor). So it is probably a good idea to also let some automated tools like Lint inspect your code and be even more paranoid about the warnings they give you. – Andy Prowl Jan 25 '13 at 15:50
  • I find that removing all warnings is worth it. I recently sat down and went through a pile of warnings on a 330k lines code base (not all of them; I disabled the "unused parameter" warning). In the end, this turned up 3 or 4 real (and quite obscure) bugs (using uninitialized memory as a source for data and sign extension bugs.) – Nikos C. Jan 25 '13 at 15:50
  • 1
    "Good code produces no warnings" isn't the same as "code that produces no warnings is good". – jthill Jan 25 '13 at 16:00
  • @R.MartinhoFernandes: On the contrary, the most important to remove are the little simple ones, because that is where you can remove the most in the least time, and then with a smaller set you can focus on those warnings that point to real issues. I believe in compiling without warnings, so I would continue until all are removed, but the first step is getting to see the important ones. – David Rodríguez - dribeas Jan 25 '13 at 17:01
  • @NikosC. - The unused parameter warning is easy to disable in a lot of code with a simple tweak. In the function definition, leave in the type name for the parameter, but omit the variable name. That shuts up the 'unused parameter' warning for that parameter for most compilers I've worked with. Most of the really pedantic warnings have simple code tweaks that you can use to signal intent and silence them. Really good compilers have warnings if you use these code tweaks when not using them wouldn't result in a warning to prevent you from just always coding in a certain way. :-) – Omnifarious Jan 25 '13 at 18:10
  • 1
    @Omnifarious This was in C code, so that doesn't work. Of course you might ask why write a function that takes a parameter that you don't use. The reason is that some parameters are there for specific builds and do nothing if specific macros are or are not defined. With C++ of course I always omit naming parameters I don't use :-) – Nikos C. Jan 25 '13 at 18:56
  • @NikosC.: I so very rarely program in straight C anymore. Is it really the case that you're not allowed to omit the names in the definition? How odd. The standards committee should add that to the next version of the standard. – Omnifarious Jan 25 '13 at 18:59
  • @Omnifarious It looks like the rationale of C++ allowing this are overrides of virtual functions, where you might not be interested at all in some of the parameters. You don't have that in C, so they probably saw no reason to introduce it. – Nikos C. Jan 25 '13 at 20:03
  • @NikosC. - That does make sense. Though C has a very similar rationale. A lot of C interfaces allow you to set hook or callback functions. And they will be given whatever parameters the thing calling them deems appropriate, which may not all be parameters that are useful when you actually write the function being called. A very similar situation to virtual functions. But anyway, here is not the place to go on about that. – Omnifarious Jan 25 '13 at 20:09

3 Answers3

13

Once you let compiler warnings slip, the 'real' warnings also get ignored as well. I can't count the number of times I've come onto a project with a ton of warnings, that with just a little care, were removed along with a bunch of very subtle bugs. Accidental assignment in an if, accidental fall-through in a switch or no default, accidental ; at the end of a for loop, unintentional type-cast etc. The warnings are there for a reason - use them. Yes, some are very pendantic, but just a little bit of work saves a lot of headache later on.

Clean the warnings and keep them clean. You will write better code.

Michael Dorgan
  • 12,453
  • 3
  • 31
  • 61
4

It can be quite tricky to fix all warnings in a large project. And some compilers have rather obscure sense of "right things to warn for".

It is definitely a good thing to fix warnings. But it should be done the right way - and sometimes, the correct thing is to simply disable that warning.

Some really annoying ones are "unused parameters" - well, if the interface determines that this function is called with two parameters, but you only use one (or none) because for what this particular implementation does isn't using those parameters, then it's hard to fix the warning [you can of course remove the name of the varivable (or put it as a comment)].

Similarly, sometimes compilers can be so picky that it's impossible to be right - I've had cases where not having a return at the bottom of the function gives "not all paths lead to a return" and when I add a return at the bottom of the function, it says "unreachable code" - well, how do you want it? This is where (for that section of code) disabling the relevant warning is the right thing to do.

It gets worse if you have to compile the code with multiple compilers - sometimes what is "good" in compiler, turns into something that is a warning in another, and the fix for that warning turns into a warning in the next compiler, which can be tricky to solve.

Once you have a strategy for coping with warnings, and have removed the ones that you want to remove, disabled the ones you want to disable, I would suggest building your product with "tread warnings as errors".

Most of the time, warnings are "good" for the programmer, and should not be ignored. But there are times when "I know what I'm doing, shut up compiler!" is the right approach - of course, most of the time, those are fixed by applying a cast or some such - and that should be done when it's the right way to fix the problem.

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227
  • You should file a bug on the compiler that has either 'unreachable code' or 'not all paths lead to a return'. If it's smart enough to know one it should be smart enough to know the other, or purposely blind enough to not give the warning. – Omnifarious Jan 25 '13 at 18:13
  • 1
    I believe this was gcc 2.95.3 or something along those lines. So I'd expect it's been fixed by now... ;) – Mats Petersson Jan 25 '13 at 18:16
3

No. Not all warnings, and not be any means.

Not all warnings: some compilers have implemented stylistic or inaccurate warnings, it's often better to deactive those warnings as they distract from real issues.

Not by any means: warnings are used by the compiler to signal something suspicious going on, the most interesting warnings signal possible undefined behavior for example. The goal should not be to remove the warning just to appease the compiler, it should be to make sure that the behavior is well-defined, and if it is not to make it well-defined.

My advice is to first review the set of activated warnings, and prune the useless ones. Then, to understand what the warnings left are about, and to agree on the best way to deal with each (with the goal of getting defined behavior). And finally you'll be able to get to work on your codebase.

Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
  • Since our libs depend on many different third party libs (boost, qt, TAO, gsoap, etc.), ends up that we have to use many different int types and such as size_t, int, int32, etc, it is really inevitable to have warnings in such case, should we just use explicit cast to force to conversion, or just leave the warning there, really not sure. – nowgains nowpains Jan 25 '13 at 16:05
  • 2
    You should avoid comparing `signed` and `unsigned` and you should avoid possible truncation. `boost::numeric_cast<>` assists in *safe* casts (it checks the value of the target and throws on underflow/overflow). – Matthieu M. Jan 25 '13 at 16:09
  • 2
    If you determine a warning isn't useful and disable it that counts as removing the warning. The point of requiring builds to produce zero warnings or using `-Werror` is so that new warnings get noticed and dealt with instead of letting 'unimportant' warnings pile up until they hide real issues. So I would say "Yes, all warnings." – bames53 Jan 25 '13 at 16:44