views:

3049

answers:

18

I've worked on many projects where I've been given code by others to update. More often than not I compile it and get about 1,000+ compiler warnings. When I see compiler warnings they make me feel dirty, so my first task is to clean up the code and remove them all. Typically I find about a dozen problems like uninitialized variables such.

I don't understand why people leave them in and don't have perfectly clean compiles with no warnings. Am I missing something? Is there any valid reason to just leave them? Any horror stories to share?

+64  A: 

I would clean up any warning. Even the ones that you know are harmless (if such a thing exists) will give a bad impression of you to whoever will compile the code.

It one of the "smelly" signs I would look for if I had to work on someone else code.

If not real errors or potential future issues, it would be a sign of sloppiness

Remo.D
+1 to this post in general. The only thing I would add is that with enough "harmless" compiler warnings, it creates a lot of "noise" that can hide more dangerous warnings.
Nick
Right. You investigate all of them, fix the broken ones. So that any new ones aren't ignored, you have to silence the harmless ones.
wnoise
Right on. I consider any warnings "visual noise" that suck away my attention from important stuff, so I kill them all when they crop up, even the ones that we all say "I know, I know, it's not a problem...."
Dan
I have encountered third-party libraries or stack especially in embedded environments with lint errors I was not allowed to cleanup. The result was a interger overflow that went through to production and became a show stopped. So cleanup as much if you can and if the politics allow it.
MeThinks
Yes, this. Not that long ago I didn't bother, but working on a large amount of code, it's clearly better: if you don't, new warnings are worthless.The time it most bugs me is if I'm in the middle of writing something and have a variable which isn't used _yet_. Of course, sometimes it's impractical: if there are already 1000s of warnings, going through and correcting them is a good thing, but it may not be able to take top priority.
A: 

I dislike warnings. I remove them as much as possible.
Sometimes, when under pressure to finish the job, i leave some of them. I rarely leave them though. I feel like you, dirty if there are any left.

Burkhard
+27  A: 

At my work, the compiler setting to treat warnings as errors is turned on. So, no warnings, or it won't compile :)

Nemanja Trifunovic
As it should be for *any* project.
Andrew
On windows that means you can't use MFC and would have to change all the 'C' standard lib functions to MS's s_versions
Martin Beckett
Yes. However, I must add that for my personal projects I simply #pragma away the security warnings. _s versions are not really safe and are unportable and slow.
Nemanja Trifunovic
+5  A: 

Always clean up your warnings. If you have a specific case where you know the warning is ok, then suppress it for that instance only.

While some warnings can be benign, most signify a real problem with the code.

If you don't clean up all of your warnings then the warning list will continue to grow and the real problem cases will be lost in a sea of warning noise.

17 of 26
A: 

The codebase I work on has over 4000 warnings. Some of them are legit problems. We never are given time to go in and fix them, nor refactor other broken things... Partly, this is because the code is so old it predates standardized C++. We can only compile in VC++ 6.

rmeador
+4  A: 

One of the characteristics of a really good programmer is that bad code gives them a queasy stomach.

I strive to have all of my code not only compiler clean, but also be clean inside my IDE set to a fairly picky level. I'll sometimes need to suppress a warning instance if I know better than the tool, but at least that also serves as documentation.

Darron
+6  A: 

The worst part is that when you write new code, its hard to know if you've accidentally introduced more warnings, since there are so many you just ignore them anyways.

The problem with cleaning them all up, is that it takes time that you may or may not have. But yes, generally you should clean up as many as you can.

Greg Rogers
+37  A: 

Clean 'em up, even if they don't indicate a real problem. Otherwise, if a warning that does indicate a real problem shows up, you won't see it through all the noise.

Graeme Perrow
+1  A: 

Warnings are and should be treated as bugs. If you can't code well enough to get rid of your warnings then you probably shouldn't be coding. In my group we made the decision to force all warnings to errors. It ends this discussion altogether and really, IMHO, improves code quality.

JaredPar
A: 

Always clean up all warnings or explicitly suppress them if needed. The default settings for warnings should be highest possible when compiling (level 4 on VS for example).

Dan Cristoloveanu
+15  A: 

I agree it's best to eliminate all warnings. If you're getting thousands of warnings you should prioritize your fixes.

Start be setting your compiler to the lowest warning level. These warnings should be the most important. When those are fixed, increment your warning level and repeat until you reach the highest warning level. Then set your compile options such that warnings are treated as errors.

If you find a warning that you suspect is safe to ignore, do some research to verify your theory. Only then disable it and only in the most minimal way possible. Most compilers have #pragma directives that can disable/enable warnings for just a portion of a file. Here's a Visual C++ example:

typedef struct _X * X; // from external header, not 64-bit portable

#pragma warning( push )
#pragma warning( disable: 4312 ) // 64-bit portability warning
X x = reinterpret_cast< X >( 0xDDDDDDDD ); // we know X not 64-bit portable
#pragma warning( pop )

Note that this only disables the warning for a single line of code. Using this method also allows you to do simple text searching of your code in the future to make changes.

Alternatively you can usually disable a particular warning for a single file or for all files. IMHO this is dangerous and should only be a last resort.

jwfearn
I'd personally prefer to see a comment next to that #pragma warning(disable: 4312) so that I didn't have to go and look up the warning number to know what it was... If it's not obvious from the description of the warning then I'd also like a reason for why it's safe to disable it.
Len Holgate
good point, I actually removed the comment for brevity, I'll add it back in
jwfearn
also - just as when fixing errors, often fixing a couple near the top fixes a slew of cascaded ones down below :)
warren
+11  A: 

Clean them up if possible. On a multi-platform/multi-compiler codebase (I've worked on one that compiled on 7 different OSs with 6 different compilers) that's not always possible though. I've seen cases where the compiler is just wrong (HP-UX aCC on Itanium, I'm looking at you), but that's admittedly rare. As others note, you can disable the warning in such a situation.

Many times what's a warning in this version of the compiler may become an error in the next version (anyone upgrading from gcc 3.x to 4.x should be familiar with that), so clean it up now.

Some compilers will emit really useful warnings that will become problems under certain circumstances -- Visual C++ 2005 and 2008 can warn you about 64-bit issues, which is a HUGE benefit nowadays. If you have any plans to migrate to 64-bit, just cleaning up those kinds of warnings will dramatically reduce your port time.

Zathrus
+6  A: 

There are some instances where I will leave warnings in code, or where it's infeasible to clean them up (although I do remove the ones I can). For example:

  • If you have something you're working on, and you know it needs more work/attention, leaving a warning in place to indicate this can be appropriate
  • If you're compiling C++ with /clr, there are several warnings about things which cause native code to be generated; it can be cumbersome to suppress all these warnings when the codebase cannot be functionally changed
  • Cleaning up warnings when you don't understand what the fix does. I've done this a couple times with PC-Lint warning, and ended up introducing bugs. If you don't know what the exact effect of the change is (eg: C-style casts to eliminate warnings), DO NOT do it. Figure out the warning, or leave the code alone is my advice.

Anyway, those are the instances off the top of my head where leaving warnings might be appropriate.

Nick
i am voting this up because Nick mentioned the following HUGELY important case when you should not mess with warning-flagged code: "Cleaning up warnings when you don't understand what the fix does"
que que
+2  A: 

I have worked with a number of embedded systems where the warnings will result in instability, crashing, or memory corruption. Unless you know the warning is innocuous, it should be dealt with.

Ray
+5  A: 

Leaving warnings in your code because you don't have time to fix them is like not brushing your teeth because you don't have enough time in the morning. It is a basic matter of code hygiene.

JesperE
+2  A: 

I always enable all warnings, and then set my project to stop building if there are any warnings.

If there are warnings, then you need to check each one to ensure that there is no problem. Doing this over and over is a waste of time. Not doing this implies will lead to errors creeping into your code.

There are ways to remove a warning (e.g. #pragma argsused).

Let the compiler do the work.

selwyn
A: 

My boss who created some code I now maintain. He uses compiler flags to hide his deprication warnings.

When I have time I go through and clean up what I can.

J.J.
A: 

I try to compile code with a fairly high level of warnings and clean them all up, except for "signed / unsigned comparison" warnings, which I'm sure I should fix but can never be bothered.

Short version: In g++ I use "-Wextra -Wno-sign-compare" and get rid of all messages.

Chris Jefferson