9

I work on a C++ project that generates bajillions of warnings. Most of the warnings appeared after the code was written:

  • Initially the project used Visual C++ 8, soon switching to 9, but there is little difference in the generated warnings. Warnings were either fixed or silenced, so there were no warnings then.
  • Then a 64-bit target was added. It generated a huge amount of warnings, mainly because of sloppy use of types (e.g. unsigned vs. size_t). No one bothered, or had time, to fix them once the code worked for the purpose.
  • Then support for other platforms using GCC (4.5 and 4.6, some 4.4 initially) was added. GCC is much more picky, so it generated many more warnings. Again nobody bothered, or had time, to fix them. This was complicated by the fact that GCC didn't have a pragma to silence a warning in particular bit of code until 4.5, and according to the documentation it is still not what one would need.
  • In the mean time some deprecated warnings popped up.

So now we have a project generating thousands of warnings. And I can't even say from how many places, since even .cpp files are compiled several times by different compilers and warnings in headers get printed over and over.

Is there a best practice for cleaning up something like that? Or at least some positive experience dealing with it?

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
Jan Hudec
  • 18,250
  • 1
  • 39
  • 62

5 Answers5

14

I think warnings, which are not treated as errors, are useless. You get tons of them, therefore nobody bothers to look at them, and they miss their purpose.

My suggestion is to fix them all, and start treating them as errors.
Fixing them looks scary, but I think it can be done. A good programmer that would pick up this work will very soon figure out the way to deal with each warning, and will handle it very mechanically and quickly.

We did a similar project in my company, and it did work - we now have no warnings in the part of the code where it's important to us, and I'm talking about many thousands of files (don't know how many warnings).

This must be immediately followed by treating warnings as errors. Once a part of your code is warning-free, change the compilation flags for this part, and let no new warnings enter. If you fail to do that, you'll find new warnings pop up like mushrooms, and your work will be wasted.

One concern you may have is introducing bugs as a result of fixing the warnings. This is especially likely because the programmer fixing the warnings probably doesn't know most of the code he's changing. However, most changes would be very safe - e.g. adding a function prototype or a cast.

ugoren
  • 487
  • 3
  • 9
  • 2
    +1: "I think warnings, which are not treated as errors, are useless.": I agree, why would I want to be warned about something that is not a problem? – Giorgio Apr 02 '13 at 15:08
  • 1
    @Giorgio Because warnings don't necessarily need to be fixed, or there may not be time/money available to fix them right now. They are a form of technical debt. That doesn't mean they shouldn't be handled eventually, but it also means that they shouldn't necessarily be swept under the rug. Your only remaining option is to have them; they're there to make you itchy, so that maybe they eventually get fixed. – Robert Harvey Apr 02 '13 at 17:05
  • 1
    @Robert Harvey: I totally agree with you. The only risk is that one starts ignoring warnings and after a while they do not make you itchy any more: I have seen warnings appear for years in the same project, and every developer would say that they were supposed to be fixed "some time in the future". Nobody was paying attention to these warnings any more: they had become part of the normal output in the master build log. I am not sure how to avoid this kind of situations. – Giorgio Apr 02 '13 at 18:25
  • 2
    Problem with fixing them all "now" mentality is it leads to incorrect fixes and less maintainable code - in C++, its amazing how easily a typecast can make a warning go away, more often than not the wrong thing to do (e.g. sign/unsigned warnings). It's legacy code so unlikely to have automated regression tests, and code changes will introduce defects. "Fix em all and b.gg.r the consequences" is not the correct approach. – mattnz Apr 02 '13 at 20:09
  • @mattnz, Breaking the code while fixing the warning is indeed the main concern here. Yet, I think there's no other way - either fix them and treat future ones as errors, or ignore them (and occasionally find bugs, about which you were warned). – ugoren Apr 03 '13 at 10:26
  • Say you have a warning about signed/unsigned int conversion- Solution is to cast the unsigned int to an int. Later a change the variable to an unsigned long. That cast used to "fix" the warning is now masking an error (loss of precision). – mattnz Apr 03 '13 at 20:54
4

Don't touch any of the legacy code. Find a way to suppress the warnings for now. Making seemingly innocuous changes to the code can introduce bugs in code which, I assume, has already been tested and is relatively bug free.

By suppressing all of the warnings that are currently in the codebase, you can pretend to have a clean slate. Any new warnings introduced into the code should be eliminated, and when old code is rewritten the new implementation should not suppress warnings.

Your goals here should be:

  1. Reduce the signal/noise ratio. If developers see thousands of notices when they build, they aren't going to notice that the master branch has 2004 notices and their branch has 2006.

  2. Don't introduce any more bugs. By making the aforementioned innocuous changes, you could be introducing unforseen bugs. This is especially pertinent when you have thousands of warnings. Even if your error rate is incredibly low, you still have thousands of chances to screw up.

  3. Don't let the legacy code lower your standards for new code. Just because the warnings for a class were suppressed doesn't mean that the new class can do the same. Eventually, with diligence, the legacy code will be eliminated from the codebase and replaced with newer, warning-free code.

Jonathan Rich
  • 2,958
  • 16
  • 14
  • 1
    +1 "Making seemingly innocuous changes to the code can introduce bugs in code..." - Legacy code is unlikely to have automated testing. These bugs will not be found. – mattnz Apr 02 '13 at 20:06
  • @mattnz: Actually the little automated tests are spread across both legacy and non-legacy code. It is an interactive application, most of it can't be reasonably unit-tested. The unit tests that exist crash (on memory corruption) on the continuous integration server. For over a year already, but nobody ever got around to fixing it because it can't be reproduced under debugger. And to be useful the CI server would need to fetch some data samples to test on which was never implemented either. – Jan Hudec Apr 03 '13 at 07:20
  • 1
    Now that boils down to how to silence the existing warnings efficiently without silencing any similar warnings in the new code. – Jan Hudec Apr 03 '13 at 07:27
  • @Jan : are you really saying you can't be bothered fixing the few tests you have? On a Programming forum? Really? (SMH) – mattnz Apr 03 '13 at 07:34
  • 2
    @mattnz: Yes. Because the tests are useless. And will always be. In the most common case of business information systems, automated tests are great, but for the kind of interactive application we have they bring so little value that we only use them occasionally for debugging some support functions. Most bugs happen in code that can only be tested interactively. I can imagine writing some automation for it, but it would be a lot of work I am not sure would be cost-efficient. – Jan Hudec Apr 03 '13 at 07:39
  • Ignoring warnings on legacy code is good if this code is never touched. In this case, if it works, all is well. But if occasional maintenance is needed, you really want warning free code. – ugoren Apr 03 '13 at 10:33
  • @Jan: In that case discard them, like code that is not called any more, best not to leave it lying around making a mess for people to trip over. – mattnz Apr 03 '13 at 20:45
3

I was reading C++ Coding Standards: 101 Rules, Guideleines, and Best Practices and the second of guidelines suggests "Compile cleany at high warning levels". It outlines a number of reasons why you would not want that many warnings in a program and chief among them are

  1. There is a potential problem in your code.
  2. Should have silent, successful builds and, if not, you may be missing real problems.
  3. Benign warnings ignored could obscure warnings pointing to real dangers

Programs that compile with warnings may appear correct but it is possible there are real problems with the code that may crop up at some point. I would suggest categorizing the warnings, give them a level of prioritization and, when you have time, get them corrected.

Mushy
  • 345
  • 1
  • 7
  • +1: for "Compile cleany at high warning levels": I think we should use as much help from the compiler as we can get. – Giorgio Apr 02 '13 at 15:09
1

Either set aside a sprint/cycle to do preventative maintenance on the system, clearing out dead code, eliminating warnings, cleaning up bad comments, things like that, OR institute a policy to eliminate warnings as sources are touched anyway for something else.
I'd not do the former myself just to get rid of warnings, but if such a cycle were planned that'd be part of the workload. Reason for that is that touching a file for anything introduces risk of bugs, and thus risks the stability of your product.

jwenting
  • 9,783
  • 3
  • 28
  • 45
  • 2
    We can't afford to set aside a sprint. So eliminating warnings as sources are touched for other reasons it shall be. But there are some sources that were not touched for long time and it will take long time until they will, so we need to tell the warnings from those legacy bits. – Jan Hudec Apr 02 '13 at 07:24
  • @JanHudec might want to do an analysis into the severity of the warnings in those cases, maybe schedule some time to revisit that old code and do a cleanup on selected bits, refactor to keep it alive. Else it may get stale and people afraid to touch it even if needed because nobody is left knows how it works. – jwenting Apr 02 '13 at 08:45
  • @JanHudec - Sounds like you don't have time to resolve the warnings if you can't afford to do preventative maintenance on the system. – Ramhound Apr 02 '13 at 13:38
  • @Ramhound: I suppose we could do a bit of janitoring, but not much and it has to run in parallel with other work. – Jan Hudec Apr 02 '13 at 13:45
1

I feel your pain because I am in a similar situation. The way I approached the problem is - remove the warnings one module/subproject at the time and set the "treat warnings as error" (Werr) compiler flag for the module after it has been cleaned up. Also, I decided to focus on one platform only, even though we build on several operating systems with different compilers. Finally, for the third-part libraries that we compile, I just turn the warnings off.

Mind you, getting rid of some of these warnings is much easier said than done. For instance, size_t vs unsigned issues you mention can cause integer overflows if you just silence the warnings with casts - so in some cases you replace unsigned with size_t, in other cases you need to detect an overflow at run-time. It is all case-by-case - very tedious and hard.

Nemanja Trifunovic
  • 6,815
  • 1
  • 26
  • 34