Clang now builds Postgres without additional warnings | Hacker News Reader

Clang now builds Postgres without additional warnings

pgeoghegan.blogspot.com

94 pointsstesch19 comments

I write all my C and C++ code at warning level 4 (the maximum, which is probably equivalent to -Wall). IMO, it was a mistake for Visual Studio to default to warning level 3. Ignoring the compiler in production code is a sign of laziness, or that you don't take what you're doing seriously.

Warnings should be disabled on a per-source-file basis. And since you actively choose to ignore specific warnings, you still benefit from the compiler's other sanity checks.

In Visual Studio, this is accomplished by

  #pragma warning( disable: 4100 )

at the top of a .c or .cpp file. This will ignore "unreferenced parameter" warnings. (Personally, I don't ignore those either. It's saved me on more than one occasion, and once is enough for me. So instead, I use a macro UNREF_PARAM(param) to suppress the warning in specific cases.)

A header file sometimes generates a warning that you want to choose to ignore. But if we disabled it in the header file, then that disable would propagate to all other code which included the header file -- a big mistake!

Visual Studio solves this by:

  #pragma warning( push )
  #pragma warning( disable: 4100 )
  //----------------------------------------------------
  // foo.h
  //   The Foo module serves as an example of how to
  // properly suppress the "unreferenced parameter"
  // warning within a header file, via Visual Studio's
  // #pragma warning( push ) and pop mechanism.
  //----------------------------------------------------
  /*
    .... code which generates the warning ....
  */
  #pragma warning( pop )

This allows you to "scope" the disable to a single header file.

(Lastly, it's worth noting that SQLite outputs hundreds of warnings --- but they are neither lazy nor irresponsible, because they have hundreds of unit tests to verify that SQLite works correctly at runtime, as defined by the tests, across all platforms.)

Surely there is such a thing as a piece of code that generates very legitimate warnings, but happens to compile to something that appears to work under at least most circumstances.

Ignoring compiler warnings does seem pretty irresponsible to me, no matter how superb your unit test coverage is. At the very least it will cause a degregation of the codebase over time.

"Surely there is such a thing as a piece of code that generates very legitimate warnings, but happens to compile to something that appears to work under at least most circumstances."

Yes. The most trivial example is probably (C++):

  Foo* foo = malloc( sizeof(Foo) ); // warning: void* to Foo*

But this is entirely valid as long as Foo is a POD type, or as long as you use placement new afterwards.

The most annoying warning ever is probably:

  while ( true )  // warning: conditional expression is constant
  {
    foo();

    if ( bar() )
      break;

    baz();
  }

Thanks, compiler! <facepalm>

It's silliness like this which has caused many people to kneejerk ignore every warning ever. But in doing so, they don't realize they're throwing away a lot of free data about their codebase, while also encouraging dangerous coding practices.

A common idiom to use here would be for(;;) {...}

Truth! That dodges the warning nicely.

But check this out:

  #define SOME_MACRO( foo, bar )  \
      do                          \
      {                           \
          foo();                  \
          bar();                  \
      } while (0)  // warning: conditional expression
                   //   is constant

That's the only possible way to correctly enforce using a semicolon following a call to SOME_MACRO(a,b);

Most insidiously, you can't wrap the macro with #pragma warning (push, disable, pop) because it's a macro. So the warning can only be suppressed where you call it, not where it's defined!

That's why I globally disable this warning in all my projects. I've never been saved by it, and it's sucked away enough of my life.

Interesting, I just tried compiling this with gcc 4.4.5 and got no warnings:

  #include <stdio.h>
  #define ADD(x, y, z) \
  do { \
  z = x + y; \
  } while (0)

  int main(int argc, char *argv[])
  {
      int z;
      ADD(1, 3, z);
      printf("z = %d\n", z);
      return 0;
  }

If your compiler complains about the do{} while (0) construct in macros, I'd be inclined to treat it as a bug and complain to the vendor.

Good to know, thank you so much! Would you mind testing with -Wall etc? (And any other "give me more verbosity!" settings)

It was a poor design choice by Microsoft. All of their compilers emit the warning when using the highest warning level.

Thanks again --- if GCC never warns, then I feel much better about globally disabling it.

I don't get any warnings with -Wall and I get a "unused variables argc and argc" with -Wextra.

Changing main's signature to int main() eliminates all warnings at -Wextra.

Brilliant, thanks!

I've used up enough of your time already, but if you feel like it...:

Do you know if gcc has the equivalent of #pragma warning( disable: N ) ? i.e. could you disable your argc and argv warning for just that source file, and no other?

If yes, then is there a way to "scope" that disable to be within a header file? (#pragma warning(push) and #pragma warning(pop))

Yep, you can disable individual warnings with pragmas (http://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html) so if I add

  #pragma GCC diagnostic ignored "-Wunused-parameter"

to the C file, I get warningless compilation even with -Wextra. you can also do push/pop to store/restore the warnings reporting options.

Thank you again!

And how exactly could SQLite benefit from warning-free compilation? It not only appears to work but it does its job pretty well.

Unit tests prove a different level of semantics that these warnings cannot substitute (although possibly complement).

Tests and warnings compliment each other nicely.

"My, you're looking testy today!" ...

burgerbrain points out something very important: Code bases with warnings tend to be degenerate.

Tend to. SQLite is not one of them. But I've seen several large codebases. The two which were beautiful also had zero warnings, and the ~dozen others ranged from "meh" all the way down to "lol, grab a snickers".

This is obviously not enough data to draw any meaningful statistical conclusions from --- but it would be silly to ignore it completely. Good C/C++ programmers tend to respect compiler warnings.

> Unit tests prove a different level of semantics that these warnings cannot substitute (although possibly complement).

That, actually, is part of the point. Unit tests check for correct expression of the problem domain (and solution). Compiler warnings check for correct expression of language constructs. It's possible, though not usually likely, for incorrect language usage to mask incorrect implementations.

I've switched my FreeBSD machine over to clang and have been successfully building most ports with it. It really is ready for prime-time :-).

If I remember correctly from this and last years WWDC, large parts (if not all by now) of iOS and OS X Lion are built with Clang/LLVM and have been for some time. I'd say Clang's been ready for a while!

Speaking of that I've played with it in the past for building things, but haven't in about a year. How is it doing performance wise these days?

Great. But what does that mean to Postgres users?

Does it affect the performance? Would it be easier to uncover more bugs moving on?

Right now it's more useful for uncovering bugs, I don't think anyone would recommend running Postgres compiled with clang on a production system.

For more info about performance, see http://pgeoghegan.blogspot.com/2011/07/could-clang-displace-...

Reply on news.ycombinator.com