836073 - Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Peterson [:cpeterson]]

Bug 833254 added a MOZ_GCC_VERSION_AT_LEAST() macro to simplify gcc version checks. In bug 833254 comment 5, Waldo suggested adding a similar macro for MSVC.

Comment 1

[Biru [:poiru]]

Attachment: Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

Comment 2

[Biru [:poiru]]

Attachment: Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 775831 [details] [diff] [review]
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

Review of attachment 775831 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.  Although, I find myself wondering: should our macros take the MSVC version number as exposed by _MSC_FULL_VER, or should they use MSVC8, MSVC9, MSVC10, MSVC11 number?  The latter might be more readily understood/usable by developers.  I know I certainly only think about MSVC versions in terms of either 2005/2008/2010/etc. or as 8/9/10/11.  Am I unusual in that regard?  Feedback from other developers appreciated on the point.

Given that I don't think there's any way to access 2005/2008/2010/etc. numbers, and possibly those are just confusing, I suspect we'd want this in terms of 8/9/10/11 numbering.  I don't know if there's a correlation between the internal MSVC version and the external 8/9/10 number, tho.  If there's none, I guess we really would be stuck with using the _MSC_FULL_VER numbers.

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Seems reasonable.  Although, I find myself wondering: should our macros take
> the MSVC version number as exposed by _MSC_FULL_VER, or should they use
> MSVC8, MSVC9, MSVC10, MSVC11 number?  The latter might be more readily
> understood/usable by developers.  I know I certainly only think about MSVC
> versions in terms of either 2005/2008/2010/etc. or as 8/9/10/11.  Am I
> unusual in that regard?  Feedback from other developers appreciated on the
> point.

I'd prefer the 8/9/10/11 numbers over trying to figure out what 0x1400 refers to.

Comment 5

[(no longer active)]

Note that we currently only support building with VC10 and 11.

Comment 6

[Biru [:poiru]]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> I'd prefer the 8/9/10/11 numbers over trying to figure out what 0x1400
> refers to.

Yeah, agreed. Here is a list of version numbers I managed to scavenge through Google (not sure if they are accurate):

                _MSC_VER    _MSC_FULL_VER
VC6 SP5           1200        12008804
VC2002            1300        13009466
VC2003            1310        13103077
VC2003 SP1        1310        13106030
VC2005            1400        140050320
VC2005 SP1        1400        140050727
VC2008 RTM        1500        150021022
VC2008 SP1        1500        150030729
VC2010 RTM        1600        160030319
VC2010 SP1        1600        160040219
VC2012 RTM        1700        170050727
VC2012 NovCTP     1700        170051025

Perhaps we could have some convinience macros such as MOZ_MSVC_VERSION_AT_LEAST_2010, MOZ_MSVC_VERSION_AT_LEAST_2010_SP1, etc.?

Comment 7

[Jeff Walden [:Waldo]]

We can always do arithmetic to match 1200 and such to 8/9/10/11, in a big huge ternary expression.  Not ideal, but certainly doable.

#define MOZ_MSVC_VERSION_AT_LEAST(version) \
  (version == 10 ? _MSC_VER >= 1600 : \
   version == 11 ? _MSC_VER >= 1700 : \
   0)

Or something like that.

It looks like we almost never test _MSC_FULL_VER, only _MSC_VER, so differences between SPx and RTM are almost always irrelevant.  I don't know if that's always going to be the case.

Comment 8

[Chris Peterson [:cpeterson]]

Assuming:

MSVC    MSVC    _MSC_VER
2002    7       1300
2003    7.1     1310
2005    8       1400
2008    9       1500
2010    10      1600
2012    11      1700
2013    12      ????

https://en.wikipedia.org/wiki/Microsoft_Visual_Studio#Version_history

The following one-liner could work for VC7 and up (but without differentiating between 7.0 and 7.1), but it might not be future-proof:

#define MOZ_MSVC_VERSION_AT_LEAST(version) ((version) >= (((_MSC_VER) / 100) - 6))

Comment 9

[Biru [:poiru]]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> #define MOZ_MSVC_VERSION_AT_LEAST(version) ((version) >= (((_MSC_VER) / 100)
> - 6))

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> #define MOZ_MSVC_VERSION_AT_LEAST(version) \
>   (version == 10 ? _MSC_VER >= 1600 : \
>    version == 11 ? _MSC_VER >= 1700 : \
>    0)

Which method should we settle on?

Comment 10

[Xavier Fung]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> Assuming:
> 
> MSVC    MSVC    _MSC_VER
> 2002    7       1300
> 2003    7.1     1310
> 2005    8       1400
> 2008    9       1500
> 2010    10      1600
> 2012    11      1700
> 2013    12      ????
> 
> https://en.wikipedia.org/wiki/Microsoft_Visual_Studio#Version_history
> 
> The following one-liner could work for VC7 and up (but without
> differentiating between 7.0 and 7.1), but it might not be future-proof:
> 
> #define MOZ_MSVC_VERSION_AT_LEAST(version) ((version) >= (((_MSC_VER) / 100)
> - 6))

The _MSC_VER for MSVC2013/12 is 1800.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Note that we currently only support building with VC10 and 11.

And VC12 experimentally.

Comment 12

[Biru [:poiru]]

Waldo, any thoughts on comment 9?

Comment 13

[Jeff Walden [:Waldo]]

Comment 7 is future-proof.  Comment 8 is not (and maybe buggy if comment 10 is to be believed, but I'm not going to research it).  Seems an easy choice.  :-)

Comment 14

[Biru [:poiru]]

Attachment: Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

(MSVC needs those extra parentheses for some reason.)

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 8443549 [details] [diff] [review]
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

Review of attachment 8443549 [details] [diff] [review]:
-----------------------------------------------------------------

If parenthesizing just _MSC_VER in that expression works as well, I'd mildly prefer that for readability, but it doesn't much matter.

Comment 16

[Biru [:poiru]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=44b65ea7cb6e

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a7ce129020

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43a7ce129020