515064 - compilation warning with MinGW32 in pratom.h

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Guenter]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.13) Gecko/2009080200 SUSE/3.0.13-0.1.2 Firefox/3.0.13
Build Identifier: NSPR 4.8.0

Since MinGW32 also defines _WIN32 line 113 of pratom.h gives this warning:
include/nspr4/pratom.h:113:71: warning: "_MSC_VER" is not defined
the following simple patch fixes this warning:
--- pratom.h.orig       2009-07-01 00:11:46.000000000 +0200
+++ pratom.h    2009-09-07 16:51:39.000000000 +0200
@@ -110,7 +110,7 @@
 **    the macros and functions won't be compatible and can't be used
 **    interchangeably.
 */
-#if defined(_WIN32) && !defined(_WIN32_WCE) && (_MSC_VER >= 1310)
+#if defined(_WIN32) && !defined(_WIN32_WCE) && defined(_MSC_VER) && (_MSC_VER >= 1310)

 long __cdecl _InterlockedIncrement(long volatile *Addend);
 #pragma intrinsic(_InterlockedIncrement)

IMO the first check for _WIN32 can be ommited because the check for _MSC_VER should be sufficient.


Reproducible: Always

Steps to Reproduce:
1. compile a source which includes nspr.h with MinGW32.

Actual Results:  
include/nspr4/pratom.h:113:71: warning: "_MSC_VER" is not defined


Expected Results:  
no warnings.

Comment 1

[Wan-Teh Chang]

Thanks for the bug report and the patch.  It is legal to use
an undefined macro in a conditional expression like
"MSC_VER >= 1310".  The undefined macro gets the value 0L.

Do you know why MinGW32 warns about this?  Are you compiling
with a very strict warning flag?

Comment 2

[Guenter]

Hi Wan-Teh,
(In reply to comment #1)
> Do you know why MinGW32 warns about this?  Are you compiling
> with a very strict warning flag?
yes, -Wundef triggers it; but if I develop I think its a valid desire to see as much warnings as possible; sure I know that I can ignore the warning in this case. But think of doing autobuilds of a project from cvs / svn / git repository - then you want to see if any recent commits made something break, and then you want to get an automatic message if the autobuild was not warning-free. This is currently not possible with an app which is a NSS consumer.

thanks, Günter.

Comment 3

[Wan-Teh Chang]

Thanks for the info.  I checked the GCC 4.4.1 manual:
http://gcc.gnu.org/onlinedocs/gcc-4.4.1/gcc/Warning-Options.html#Warning-Options

-Wundef is not turned on by either -Wall or -Wextra.
So it is a rarely used compiler flag.  Are you sure
you want to use -Wundef?  We write
  _MSC_VER >= 1310
intentionally, taking advantage of C preprocessor's
rule of giving an undefined macro a value of 0L in
an #if conditional expression.  I am sure we're not
alone.

-Wundef also warns about the following code:

#if HAVE_FOO
extern int foo(void);
#endif

I think this kind of code is also common (we write
#ifdef HAVE_FOO for such code).  So I think you'll
get a lot of warnings if you turn on -Wundef.

In summary, I will check in your patch, but this
kind of code is likely to creep back in, and I
suggest that you not compile with -Wundef.  Do
you control the makefile that uses -Wundef?

Comment 4

[Guenter]

Hi Wan-Teh,
(In reply to comment #3)
> So it is a rarely used compiler flag.  Are you sure
> you want to use -Wundef?
yes. It is very useful to detect typos.
> We write _MSC_VER >= 1310
> intentionally, taking advantage of C preprocessor's
> rule of giving an undefined macro a value of 0L in
> an #if conditional expression.  I am sure we're not
> alone.
from what I currently see you are: no other SSL library, and no kernel headers have such; and the reason why you dont get more BZs is simply because this problem only occurs with MinGW32, and no other compiler; I bet that you would get BZs about such a thing if it would be triggered on any Linux platform.

> -Wundef also warns about the following code:
> 
> #if HAVE_FOO
> extern int foo(void);
> #endif
correct, and that is also bad coding style - instead use:
#if defined(HAVE_FOO) && (HAVE_FOO > 0)
if you want to test for the value of HAVE_FOO ...

> I think this kind of code is also common (we write
> #ifdef HAVE_FOO for such code).  So I think you'll
> get a lot of warnings if you turn on -Wundef.
nope, only the one in the NSPR header I mentioned in this BZ so far.

> In summary, I will check in your patch, but this
thank you!
> kind of code is likely to creep back in, and I
no prob - I file another BZ then :)
> suggest that you not compile with -Wundef.  Do
> you control the makefile that uses -Wundef?
well, I'm member of a couple of projects, and such things would then be proposed, and commonly decided - but since I personally dont agree that removing -Wundef is a good idea since it helped already in the past to catch macro typos I will not suggest such a change.
What downside do you see to let the pre-processor first check if a macro is defined before you check for its value?
Also it might the true for gcc that undefined macros are automatically assigned 0L - but are you 100% sure that this is true for each and every compiler in the wild?

Again, thanks for changing it.

Comment 5

[Wan-Teh Chang]

The default value of 0L for an undefined macro is specified
by the C language.  For example, in K & R's "The C Programming
Language," 2nd Ed., it is specified in Section A 12.5 on page
232:
  Any identifiers remaining after macro expansion are replaced
  by 0L.

We write expressions like
  (_MSC_VER >= 1310)
because they're more concise than
  defined(_MSC_VER) && (_MSC_VER >= 1310)
Sometimes, this allows us to stay within 80 characters and avoid
wrapping a long line.

Comment 6

[Wan-Teh Chang]

Attachment: Guenter's patch

I checked in this patch on the NSPR trunk (NSPR 4.8.1).

Checking in pratom.h;
/cvsroot/mozilla/nsprpub/pr/include/pratom.h,v  <--  pratom.h
new revision: 3.12; previous revision: 3.11
done