compilation warning with MinGW32 in pratom.h

RESOLVED FIXED in 4.8.1

Status

defect
--
trivial
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: gk, Assigned: wtc)

Tracking

other
4.8.1
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
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?
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.
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.
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
Attachment #400316 - Flags: review+
Severity: normal → trivial
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.1
You need to log in before you can comment on or make changes to this bug.