[llvm-commits] [PATCH] Fix bug with _MSC_VER define

[John Garvin]

On Aug 29, 2012, at 12:14 AM, John Garvin <jgarvin at apple.com> wrote:

> 
> On Aug 15, 2012, at 1:52 PM, Chris Lattner <clattner at apple.com> wrote:
> 
>> 
>> On Aug 15, 2012, at 11:13 AM, John Garvin <jgarvin at apple.com> wrote:
>> 
>>> It's not strictly necessary, but it would allow compiling with -Werror -Wundef.
>> 
>> Ok, is this the only sort of thing that needs this, or is it pervasive throughout the llvm headers?
>> 
>> -Chris
>> 
> 
> 
> With respect to _MSC_VER, all the uses of it, except the one fixed in the patch, are in a context where the symbol is already defined. With respect to other symbols, I'm not familiar enough with the code yet to be totally sure which symbols are defined elsewhere, but I ran clang -fsyntax-only -Wundef on each header file and grepped for the -Wundef warning, and this was the result:
> 
> include/llvm-c/Core.h:2690:9: warning: 'DEBUG' is not defined, evaluates to 0 [-Wundef]
> include/llvm/Support/Compiler.h:27:9: warning: '_MSC_VER' is not defined, evaluates to 0 [-Wundef]
> include/llvm/Support/FileSystem.h:43:5: warning: 'HAVE_SYS_STAT_H' is not defined, evaluates to 0 [-Wundef]
> include/llvm/Support/FileSystem.h:599:5: warning: 'LLVM_ON_WIN32' is not defined, evaluates to 0 [-Wundef]
> 
> About twenty more -Wundef warnings are there if you count .h files not in include/ .
> 
> Do we want to make sure all the headers in include/ compile without warning under -Wundef? It seems like a good idea to me.
> 
> John

This patch makes sure preprocessor macros in the include subdirectory are not used without being defined. This adds to the previous patch, which only made this change for the _MSC_VER macro.

Rationale: For each preprocessor macro, either the definedness is what's meaningful, or the value is what's meaningful, or both. If definedness is meaningful, we should use #ifdef. If the value is meaningful, we should use #if and it should be an error if it's undefined. If both, we should ask #ifdef and then ask about the value. Mixing up these cases, e.g., using #if and #ifdef interchangeably for the same macro, seems ugly to me, even if undefined macros are zero if used.

This also has the benefit that including an LLVM header doesn't prevent you from compiling with -Wundef -Werror.

In more detail:
_MSC_VER: change "#if _MSC_VER > number" to "#if defined(_MSC_VER) && _MSC_VER > number". This matches the other uses of _MSC_VER which are used in a context where the macro must be defined.
DEBUG, HAVE_SYS_STAT_H, and LLVM_ON_WIN32: change one use of #if to #ifdef. This is to match the #ifdef's in the rest of the code. HAVE_SYS_STAT_H is either #define'd or #undef'd by configure, so it should be #ifdef. The docs say to use #ifdef LLVM_ON_WIN32.

Does this make sense? What do you think?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: preproc.diff
Type: application/octet-stream
Size: 1517 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120830/60a17f40/attachment.obj>