views:

439

answers:

5

Please examine the following code:

if (foo->bar == NULL);
   foo->bar = strdup("Unknown");

I spent the last part of three hours hunting down that leak with Valgrind, feeling very silly when I discovered the bogus ';'.

I know that the above code is valid C, however I would love for gcc to be able to tell me if I'm using a conditional as a statement.

Is there a flag that I could pass which would help spot this type of mistake in the future? It seems to me that gcc would be able to know if a conditional is useless.

IE:

if (1 == 1);
  code_that_is_always_reached_since_conditional_is_a_statement();

None of the lints take issue with this either. Valgrind is great to find these kinds of things .. but the leak was actually much later in the code than where the pointer was originally allocated.

Any help is appreciated, even "No, it doesn't do that."

Edit:

Wow, thanks for such a fast and great response! To summarize, here are your options:

  • -Wextra picks up on all kinds of things that -Wall does not, including empty / useless statements.
  • -Wempty-body picks up on useless statements, which is enabled by -Wextra (but can break older versions of gcc, works on 4.3.x)

Some people might find -Wextra annoying. You may have a comparison between types of different signedness, but you know the comparison happens only when they are the same.

i.e.

int ret;
unsigned int i;

ret = foo(bar); /* foo() is known to return a signed errno on failure */
if (ret < 0)
  return 1;

/* Enter GCC complaining that ret differs in signedness
 * (but you know it doesn't and get the urge to cast it) */
for (i = 0; i < ret; i ++)
   ...

Thanks again for the tips!

+22  A: 
/* foo.c */
int main() {
   if (1) ; 
   return 0;
}

gcc -Wextra -c foo.c
foo.c: In function ‘main’:
foo.c:2: warning: empty body in an if-statement
Employed Russian
Wonderfully laconic.
Crashworks
I love it that -Wall doesn't mean -Wall. It was worth losing 2 rep to learn about -Wextra.
Norman Ramsey
It's for backwards-compatibility. The documentation is very clear (even repeating itself) on this point. Wall says it activates, "All of the above -W options combined.", "This enables all the warnings about constructions that some users consider questionable, and that are easy to avoid" and "The following -W... options are not implied by -Wall.", etc. Further, many warnings reiterate whether Wall activates it.
Matthew Flaschen
@Matthew - The documentation is also almost impossible to navigate because it's so massive. Perhaps they should repeat themselves a little less...
Chris Lutz
Thanks for this! I also picked up on half a dozen comparisons between types of different signedness, one of which was dangerous. I also had a 'false;' instead of 'return false;' , which -Wall did not pick up.
Tim Post
It's mostly massive because there are so many options. They could break it up into sections (different man page for each section), but for those of us with decent pagers that actually becomes /more/ work. For instance, zsh does this, and I find it annoying.
Matthew Flaschen
there are also some useful error flags not included in -Wall and -Wextra ; see http://stackoverflow.com/questions/432835/how-do-you-ensure-that-you-as-programmer-have-written-quality-c-code/432852#432852 for what I use to compile C code with gcc
Christoph
@Chris Lutz: Agreed, almost every really useful switch in gcc is an easter egg, though it remains one of the best documented compilers ever written.
Tim Post
+6  A: 

Try -Wextra

JimG
+6  A: 

After digging deep into the gcc manual:

-Wempty-body
    Warn if an empty body occurs in an `if', `else' or `do while' statement. This warning is also enabled by
-Wextra.

As some other posters wrote, -Wextra should do it

Sample code:

int main(){

        if (0);
                printf("launch missiles");
        return 0;
}


$gcc -Wempty-body foo.c
warn.c: In function ‘main’:
warn.c:5: warning: suggest braces around empty body in an ‘if’ statement
Tom
+1, its nice to be able to squelch other stuff that -Wextra picks up on, such as comparisons between two things of different signedness (when you know damn well the value of both is unsigned), i.e. the check is not reached if one of the values is signed.
Tim Post
The only problem is, this breaks earlier versions of gcc (4.2.x or earlier).
Tim Post
+3  A: 

As an alternative to the compiler, I find that running an autoindenter over the code helps find these situations.

In vim for example:

gg=G
sharth
A: 

In addition to the above, if you find yourself getting frustrated hunting for a bug using valgrind or a similar execution profiler, you should perhaps consider using a static analysis tool, such as lint. Personally, I use PC-LINT, which catches all sorts of these types of bugs.

Shane MacLaughlin
the OP said that lint didn't pick up this problem... do you believe it can?
hhafez
Just checked, PC-Lint catches it, albeit as a missing 'else'. I use VC++ which always catches it, hence I have never actually encountered it first hand. The point I'm making is that where dynamic analysis (valgrind, boundschecker, etc..) is drawing a blank, static analysis may show results. The reverse is equally true of course. I find it worthwhile to lint all my code for easily made snafus.
Shane MacLaughlin
PC-Lint combined with the MISRA-C rules file would certainly pick this up as it would complain about the lack of braces after the 'if' statement.
Al
A lack of braces after an if statement can be valid C89/99 , which is what I use. The lintian I use is called splint, it does a pretty good job of picking up things that the compiler did not. For instance, I use the -Wno-unused flag, since I know I have some static functions that have not been wired together yet, but this also squelches warning for unused variables. So, splint picks that up, and more. smaci is correct in that there has to be a good combination of lints and compiler flags, especially when you set your compiler to treat warnings as errors. I'm still brewing that magic recipe :)
Tim Post