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)

Product:
Core
Component:
MFBT
Platform:
All
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: cpeterson, Assigned: poiru)

References

Details

Attachments

(1 file, 2 obsolete files)

Add MOZ_MSVC_VERSION_AT_LEAST() macro to mfbt/Compiler.h
2.08 KB, 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.
No longer blocks: 833254
Depends on: 833254
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #775825 - Flags: review?(jwalden+bmo)
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.
Note that we currently only support building with VC10 and 11.
(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.
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))
(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?
(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.
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)
(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
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.

Attachment

General

Creator:
Created:
Updated:
Size: