The Old Joel on Software Forum: Part 4 (of 5)

Compiler warnings

A recent thread about how good Internet Explorer was at rendering invalidated HTML, prompted me to ask about warnings from the compiler. I was working with a clients' developer the other day who said that warnings from the compiler could be ignored because the program would build and run anyway.

Is this normal practice? What do you guys do if you get compiler warnings?

Interaction Architect
Saturday, April 17, 2004

I have my compiler set to treat warnings as error messages, warnings are there for a good reason and shouldn't be ignored.

Tony Edgecombe (www.frogmorecs.com)
Saturday, April 17, 2004

I always go for zero warnings. It is too easy to ignore warnings, only to find some of them actually indicated a problem.
I've enforced zero-warnings as a rule for my programmers.

Viking 147
Saturday, April 17, 2004

We always compile at the highest warning level. No production software gets released with warnings in it (without special permission from me).

Warnings are the compiler's way of saying "this bit of code looks wrong, you better check it out". A warning is a bug in waiting. We make mission-critical software for embedded systems, so bugs are a bad idea.

Also, our software (low-level libraries) ships with several example programs. We give the source for these examples. We expect the user to compile them. Warnings make the code look "sloppy".

At some point, I'd like to start using a more serious tool for syntax-checking, something like Lint.
http://www.gimpel.com/html/products.htm

Anyone used it?

Myron A. Semack
Saturday, April 17, 2004

We use a number of tools as part of our build system (java):
- the jikes compiler, which produces semantic warnings (eg, throwing the Exception superclass, rather than a subclass)
- a complete unit test suite,
- an acceptance test suite,
- clover, which tests the number of lines of code touched by the unit test suite, versus the total number of lines of code. We try to maintain > 90%,
- simian, which tests code duplication against the source code,
- checkstyle, which checks the style

The reports generated by these tools are published on the intranet whenever a build occurs on the build machine. We do more than daily builds, the build machine does a clean checkout and rebuild every 30 minutes. We use AntHill for this, but cruisecontrol is also good.

Rhys Keepence
Saturday, April 17, 2004

We have a lot of math-heavy C functions in our legacy code and pragma out all the conversion warnings (like int to float... that's got to be the dumbest one ever).

I guess somebody could go through and put an (int) in front of all 3000 of them, but nobody has the time... and the old PhD codgers who wrote them won't change their ways.

Ron
Saturday, April 17, 2004

It seems to be best practice to compile with warnings to be reported as errors. Given that, are there good reasons for compilers to issue warnings at all (i.e. shouldn't all problems be treated as errors)?

I have used Lint, and I liked it a lot. It would be nice to see the major compilers include lint-like functionality. For C++, GCC includes the -Weffc++ compiler flag, which checks for violations of a few of the guidelines in Scott Meyers' Effective C++ book.

If you use code-checking tools like lint and code-coverage analysers, you'll learn a lot about the language and about the mistakes you've been making. Having said all that, using a language that doesn't allow you make silly mistakes is a big win.

C Rose
Saturday, April 17, 2004

There are some pretty dumb warnings out there.

GCC with -Wall will warn about:

while (iter = nextInCollection(iter)) {
...
}

basically complaining that the '=' in the conditional could be mistaken for '=='.

If you compile with -Wall -Werror, then this idiom effectively becomes unusable. Sorry, but _I_ want to be setting my coding standards, not my compiler.

David Jones
Saturday, April 17, 2004

It depends. For new code I strongly encourage people to treat warnings as errors.

Unfortunately we have a large body of legacy code (some of it K&R style C) which is not warnings clean. This tends to contaminate otherwise "new" code when we #include old header files.

We are gradually moving towards more modular code which will allow us to hide the legacy stuff in a cleaner way.

I'm not keen on "fixing" all the warnings in the existing code base as it could do more harm than good (In a previous company everybody was assigned some warnings to fix. Some people misunderstood the object of the exercise and duly commented out the offending lines, breaking the code completely. For those who did things properly, only about 3% of the warnings indicated real bugs, so in that case the cure was worse than the disease.).

MugsGame
Saturday, April 17, 2004

In most compilers you can switch off individual errors.

We have a rule that the code compiles with no warnings in MSVC + gcc.
It means some ugly code like not being able to have if =, and a lot of casts. but it stops a lot of very hard to find bugs.

Martin Beckett
Saturday, April 17, 2004

My preference is that everything be free of warnings.

But, I'm forced to admit that maintaining that state takes effort, and the number of incidents where a compiler warning has actually indicated a bug in the program are few.

If there was one (and I'm not sure any longer) it was doubtless an assignment in a boolean expression. I don't have any sympathy for David's stance unless his compiler does not have a syntax to surpress this warning - for example, a "redundant" set of parens to signal to the compiler that you really mean to do that.

Quite frankly, there are bigger fish to fry.

Danil
Saturday, April 17, 2004

David J. - you can use = in while() expressions without getting a GCC warning, just put an extra set of parenthesis around it.

while((a = b())) {} rather than while(a=b) {}

GCC is a little over-eager though. If, in a non-void function, I have a switch() statement on an enum, which returns something for all valid states of the enum, but no default case, I still get "control reaches end of non-void function." This can only happen if the enum is set to an invalid value, which I don't think the compiler should be worrying about. (it should give an error when I assign an invalid value to the enum! - although maybe that's impossible if we use the old standard of letting you equate enums to integers)

Dan Maas
Saturday, April 17, 2004

"it should give an error when I assign an invalid value to the enum! - although maybe that's impossible if we use the old standard of letting you equate enums to integers"

Metrowerks produces an error if you assign an integer to an enum without a cast. The other direction works (enum to integer) without a cast. I certainly think that makes sense. But it's always possible (due to casting) to have an enum with an invalid value.

Almost Anonymous
Saturday, April 17, 2004

> But it's always possible (due to casting) to have an enum with an invalid value.

Or by failing to initialize the enum's value.

Christopher Wells
Saturday, April 17, 2004

"Some people misunderstood the object of the exercise and duly commented out the offending lines, breaking the code completely."

How do these people gets jobs coding?

C Rose
Saturday, April 17, 2004

C Rose, are you new to "professional coding"? There's a significant contingency of copy/paste coders who don't actually seem to understand what's going on...

Brad Wilson (dotnetguy.techieswithcats.com)
Saturday, April 17, 2004

> There's a significant contingency of copy/paste coders who don't actually seem to understand what's going on...

It worked for their uni assignments and just so long as they can find an open source project to copy, they're fine.

Saturday, April 17, 2004

Several decades ago I worked at a company where a product built with something like 98 warning messages, and the documentation stated this, with a hint that if the number changed, this indicated an actual problem.

One of the tasks I took upon myself was to eliminate the warnings, so if a message showed up during a build, we knew there was a problem, instead of having to check the error count.

Dan Brown
Saturday, April 17, 2004

I've been using Splint for a month now:

http://lclint.cs.virginia.edu/

It's an open source code verifier which you can use for commerical code.

Splint is wonderful. You can choose how strict you'd like it to be, and with a few extra annotations (marked up in comments) it can do some really useful checking. Read the first couple of paragraphs of the first page of the manual to get a good idea of what it does (linked from homepage).

It takes a couple of weeks to understand a little of how Splint's internal models. Initially you'll use the wrong annotations to make warnings go away, but eventually you'll get the right ones and you'll be confident that some classes of bugs (e.g. null derefs, double frees, memory leaks) simply don't exist in your program.

Since it does static analysis it checks all the edge cases in your code, including the branches that don't get taken very often.

Recommended, especially in combination with a good unit test suite.

And you should treat warnings as low-priority errors. Released code (e.g. checked into CVS) should never generate compiler warnings when compiled with a pedantic compiler (I use gcc -Wall -pedantic). As other posters have said it's sloppy and does not inspire trust. If I see a compiler warning in someone's released code then I think:
- they probably don't understand what they've written,
- they don't care about producing polished code,
- they've probably ignored a few edge cases in their code as well.

If you're getting a warning for code that you know is correct, then _and only then_ may you _locally_ disable the warning, i.e. disable it for the single statement that is causing the problem.

Tom Payne
Monday, April 19, 2004

Y'all seen strsafe.h from Microsoft? It's shipping with VC2003. They've written safe(er) replacements for all the string functions in the C runtime library. Many of these functions are useful: They'll do things like copy a string in a range-safe way, with a guaranteed null terminator, and give you back a pointer to the null terminator (E.g. StringCchCopyEx()[1]). Great stuff.

strsafe.h does a #pragma deprecated on all the old string functions, so you get a warning (C4995, IIRC) for each one, which is great: If some bozo calls gets(), the compiler yells about it.

Here's the problem: Those old-fashioned string functions are used here and there in the STL and ATL. I can't seem to suppress those (#pragma warning) without also suppressing the same warnings in my own code, which would defeat the purpose.

[1] StringCchCopyEx is in MSDN:
http://msdn.microsoft.com/library/en-us/winui/WinUI/WindowsUserInterface/Resources/Strings/StringReference/StringFunctions/stringcchcopyex.asp

...but watch out for ppszDestEnd: MSDN neglects to mention that ppszDestEnd is not just "[out]"; if present, cchDest is ignored and (*ppszDestEnd) - pszDest is used for the buffer size instead. One natural way to use the function is for concatenating a bunch of strings into one buffer. You'd like to think you can use the same pointer for pszDest and *ppszDestEnd, and avoid Schlemiel the Painter Syndrome. But what happens is that if ppszDestEnd isn't NULL, StringCchCopyEx() ignores both cchDest AND *pcchRemaining. Instead, it subtracts pszDest from *ppszDestEnd to get the remaining space in the buffer. In the case I describe, that'll be zero.

The reasoning behind that behavior is not clear to me. I wrote a little two-line wrapper function which changes the semantics to suit myself.

Aarrgghh
Monday, April 19, 2004

Oh, anyway, the point of all that was this: I really dislike un-fixed warnings, but I fear buffer overruns more.

Aarrgghh
Monday, April 19, 2004

Ron, those math-heavy C legacy functions are vulnerable to various overflow and loss of information conditions if they're single-precision floating point.

http://research.microsoft.com/~hollasch/cgindex/coding/ieeefloat.html

Single-precision has 23 bits of significant digits, while 32-bit integer (what people normally mean by int) has 32.

Jason McCullough
Monday, April 19, 2004