Closed
Bug 836073
Opened 12 years ago
Closed 10 years ago
Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: cpeterson, Assigned: poiru)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #775825 -
Attachment is obsolete: true
Attachment #775825 -
Flags: review?(jwalden+bmo)
Attachment #775831 -
Flags: review?(jwalden+bmo)
Comment 3•11 years ago
|
||
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+
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
Note that we currently only support building with VC10 and 11.
Assignee | ||
Comment 6•11 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.?
Comment 7•11 years ago
|
||
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•11 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•11 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•11 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.
Comment 11•11 years ago
|
||
(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 13•10 years ago
|
||
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•10 years ago
|
||
(MSVC needs those extra parentheses for some reason.)
Attachment #775831 -
Attachment is obsolete: true
Attachment #8443549 -
Flags: review?(jwalden+bmo)
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•