views:

485

answers:

6

I like to force a policy of no warnings when I check someone's code. Any warnings that appear have to be explicitly documented as sometimes it's not easy to remove some warnings or might require too many cycles or memory etc.

But there is a down-side to this policy and that is removing warnings in ways that are potentially dangerous, i.e. the method used actually hides the problem rather than fixes it.

The one I'm most acutely aware of is explicitly casts which might hide a bug.

What other potentially dangerous ways of removing compiler warnings in C(++) are there that I should look out for?

+5  A: 

Well, there's the obvious way - disabling a specific warning for parts of the code:

#pragma warning( disable : 4507 34 )

EDIT: As has been pointed out in the comments, it is sometimes necessary to use in cases where you know that the warnings are OK (if it wasn't a useful feature, there would have been no reason to put it in in the first place). However, it is also a very easy way to "ignore" warnings in your code and still get it to compile silently, which is what the original question was about.

Tal Pressman
Compiler specific, of course.
bltxd
But often needed -- eg Vista SDK/DDK headerfiles throw lots of warnings so you need to disable them before importing :/
Pod
You can pragma push/pragma pop warnings so that you can disable warnings in 3rd party code without disabling the same warnings in your code.
CiscoIPPhone
"if it wasn't a useful feature, there would have been no reason to put it in in the first place": yeah, sure :-D
Gyom
Additionally, in Visual Studio some warnings can be disabled for the whole project.
MP24
+6  A: 

const correctness can cause a few problems for beginners:

// following should have been declared as f(const int & x)
void f( int & x ) {
  ...
}

later:

// n is only used to pass the parameter "4"
int n = 4;
// really wanted to say f(4)
f( n );

Edit1: In a somewhat similar vein, marking all member variables as mutable, because your code often changes them when const correctness says it really shouldn't.

Edit2: Another one I've come across (possibly from Java programmers ) is to tack throw() specifications onto functions, whether they could actually throw or not.

anon
throw() specifies that a function does NOT throw anything, and thus if it did, std::terminate() would be called.
Peter Kovacs
+1  A: 

Commenting out (or worse, deleting) the code that generates the warning. Sure, the warning goes away, but you are more than just a little likely ending up with code that doesn't do what you intend.

Vatine
A: 

The biggest risk would be that someone would spend hours of development time to solve a minor warning that has no effect on the code. That would be a waste of time. Sometimes it's just easier to keep a warning and add a line of comment explaining why the warning occurs. (Until someone has time to resolve these trivial warnings.)

In my experience, resolving trivial warnings often adds two more days of work for developers. Those could make the difference between finishing before and after the deadline.

Workshop Alex
I disagree, for the most part having multiple warnings may hide other important ones.for things such as the compiler warning of implicit casts (say, a long int was implicitly converted to a char, causing truncation), the programmer should check if this is the desired behavior, adding an explicit cast(as a statement of intent) if it was
Hasturkun
I don't agree that compiler warnings should be classed as trivial. We offer a static analysis tool, and IMHO, compilers issue a tiny percentage, say 1%, of possible warnings against code. For the most part, compilers only warn when they're absolutely sure that the code as written is probably not what the developer thinks it is.
Richard Corden
Of course, warnings need to be resolved. But do keep in mind that if you have a deadline by tomorrow, then you won't have much time to resolve all the warnings. A risk analysis will tell you about the most risky warnings that need to be solved and all other warnings can be solved in the next patch of the software, after the deadline. But missing your deadline tends to be a bigger sin than leaving warnings in your code. They are, after all, just warnings.
Workshop Alex
+2  A: 

I think it's a delicate issue. My view is that warnings should be checked thoroughly to see if the code is correct / does what is intended. But often there is correct code that will produce warnings, and trying to eliminate them just convolutes the code or forces a rewrite in a less natural way.

I recall in a previous release I had correct and solid code that produced a couple of warnings, and coworkers started complaining about this. The code was much cleaner and did what it was intented. In the end the code went on production with the warnings.

Also, different compiler versions will produce different warnings, so it becomes more pointless to enforce a "no warnings" policy when the result depends on the mood of the compiler developers.

I want to emphasize how important is to check all warnings at least once.

Btw I develop in C and C++ for embedded systems.

piotr
Of course there *can* be warnings, but, IMO, they need to be documented, i.e. why is the warning not removed. And of course every warning needs to be checked, my point was that some developers might, accidentally or otherwise, remove warnings in such ways that they actually hide the bug that the compiler was warning about or create new ones.
Makis
I mostly agree with you, but the problem I have is the "broken window" effect. Too many warnings in the build and people stop looking.
Chris Arguin
+1  A: 

I also enforce a no-warnings rule, but you are right that you can't just remove the warning without careful consideration. And to be honest, at times I've left warnings in for a while because the code was right. Eventually I clean it up somehow, because once you have more than a dozen warnings in the build, people stop paying attention to them.

What you described is not a problem unique to warnings. I can't tell you how many times I see someones bug-fix for crash to be "I added a couple of NULL checks". You have to go to the root cause: Should that variable be NULL? If not, why was it?

This is why we have code reviews.

Chris Arguin