Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: poiru)

Tracking

unspecified
mozilla33
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
No longer blocks: 833254
Depends on: 833254
(Assignee)

Comment 1

6 years ago
Created attachment 775825 [details] [diff] [review]
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #775825 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

6 years ago
Created attachment 775831 [details] [diff] [review]
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h
Attachment #775825 - Attachment is obsolete: true
Attachment #775825 - Flags: review?(jwalden+bmo)
Attachment #775831 - Flags: review?(jwalden+bmo)
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.
Attachment #775831 - Flags: review?(jwalden+bmo) → feedback+
(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

6 years ago
Note that we currently only support building with VC10 and 11.
(Assignee)

Comment 6

6 years ago
(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.?
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.
(Reporter)

Comment 8

6 years ago
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))
(Assignee)

Comment 9

6 years ago
(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

6 years ago
(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.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Note that we currently only support building with VC10 and 11.

And VC12 experimentally.
(Assignee)

Comment 12

5 years ago
Waldo, any thoughts on comment 9?
Flags: needinfo?(jwalden+bmo)
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.  :-)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 14

5 years ago
Created attachment 8443549 [details] [diff] [review]
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h

(MSVC needs those extra parentheses for some reason.)
Attachment #775831 - Attachment is obsolete: true
Attachment #8443549 - Flags: review?(jwalden+bmo)
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.
Attachment #8443549 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/43a7ce129020
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.