155666 - [PATCH] nscore.h has unreachable MSVC #defines (mutually exclusive nested #if)

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Tim]

This is from CVS trunk:

http://lxr.mozilla.org/seamonkey/source/xpcom/base/nscore.h#195 contains the
following block:
186 #if defined(_MSC_VER) && (_MSC_VER>=1100)
(... still within 186's #if)
195   #if (_MSC_VER<1100)
(...)
198   #elif (_MSC_VER==1100)
(...)
206 #endif

Obviously, the block from 195 to 197 (which #undefs a symbol) is impossible to
reach in any circumstance (barring a broken preprocessor).

I'm not 100% sure XPCOM is the right place for this; it seems like it could also
go in Build Config.

Comment 1

[Tim]

Attachment: Patch to correct the problem

Comment 2

[Boris Zbarsky [:bzbarsky]]

*** Bug 155665 has been marked as a duplicate of this bug. ***

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The only thing that's going to define HAVE_CPP_EXPLICIT for windows is the code
just a few lines above, so why isn't it sufficient to just remove the four lines:

  #if (_MSC_VER<1100)
      /* before 5.0, VC++ couldn't handle explicit */
    #undef HAVE_CPP_EXPLICIT
  #elif (_MSC_VER==1100)

and replace with:

  #if (_MSC_VER==1100)

?  Not that we support building on MSVC before 5.0 anyway...

Comment 4

[Tim]

Attachment: Reduced patch

This patch reduces the changes to the original code while achieving the same
effect.

Comment 5

[Tim]

Attachment: Patch addressing dbaron's comments

I didn't reload the bug until after posting my last patch, so I didn't see
dbaron's comment.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 90254 [details] [diff] [review]
Patch addressing dbaron's comments

This patch seems fine with me, although:
 * dougt should review
 * I question whether it's worth bothering causing everyone a full rebuild, so
if you land it, please land it the same time someone else triggers a full
rebuild.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 90254 [details] [diff] [review]
Patch addressing dbaron's comments

Add a better comment similar to what is done here:
http://lxr.mozilla.org/seamonkey/source/configure.in#2351

Make sure scc also review or sr's this.

Comment 8

[Tim]

I'm not sure I understand what I'm supposed to be commenting.

The change removes code that is impossible to reach. Additionally, it is
redundant because HAVE_CPP_EXPLICIT would only be #defined in the situation
where _MSC_VER>=1100, in which case _MSC_VER would not be <1100. If _MSC_VER
were <1100, it would have never entered the block where HAVE_CPP_EXPLICIT was
defined.

If you're asking me to comment as to why we don't define HAVE_CPP_EXPLICIT if
_MSC_VER<1100, I have no idea.  I don't know of the peculiarities of the various
versions of VC++.

Comment 9

[Scott Collins]

Comment on attachment 90254 [details] [diff] [review]
Patch addressing dbaron's comments

yeah, it's unreachable.  This patch gives the functionally reachable
equivalent.  sr=scc

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 90254 [details] [diff] [review]
Patch addressing dbaron's comments

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 11

[Doug Turner (:dougt)]

Fixed.

Checking in nscore.h;
/cvsroot/mozilla/xpcom/base/nscore.h,v  <--  nscore.h
new revision: 1.51; previous revision: 1.50
done


Thanks all.

Comment 12

[Tim]

Verified.