r211420 - Driver: enhance MSC version compatibility
Aaron Ballman
aaron at aaronballman.com
Mon Jun 23 10:48:19 PDT 2014
On Mon, Jun 23, 2014 at 1:36 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 23/06/2014 19:54, Aaron Ballman wrote:
>>
>> On Mon, Jun 23, 2014 at 12:50 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> On 23/06/2014 19:27, Saleem Abdulrasool wrote:
>>>>
>>>> On Jun 23, 2014, at 6:38 AM, Alp Toker <alp at nuanti.com> wrote:
>>>>
>>>>> On 23/06/2014 15:02, Aaron Ballman wrote:
>>>>>>
>>>>>> On Sun, Jun 22, 2014 at 10:09 PM, Alp Toker <alp at nuanti.com> wrote:
>>>>>>>
>>>>>>> On 23/06/2014 04:30, Saleem Abdulrasool wrote:
>>>>>>>>
>>>>>>>> On Sun, Jun 22, 2014 at 4:15 PM, Alp Toker <alp at nuanti.com
>>>>>>>> <mailto:alp at nuanti.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23/06/2014 01:53, Saleem Abdulrasool wrote:
>>>>>>>>
>>>>>>>> On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker
>>>>>>>> <alp at nuanti.com
>>>>>>>> <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>>>>>>>>
>>>>>>>> <mailto:alp at nuanti.com>>> wrote:
>>>>>>>>
>>>>>>>> Late to the party with review but I have some points
>>>>>>>> of
>>>>>>>> review..
>>>>>>>>
>>>>>>>>
>>>>>>>> On 21/06/2014 01:58, Saleem Abdulrasool wrote:
>>>>>>>>
>>>>>>>> Author: compnerd
>>>>>>>> Date: Fri Jun 20 17:58:35 2014
>>>>>>>> New Revision: 211420
>>>>>>>>
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project?rev=211420&view=rev
>>>>>>>> Log:
>>>>>>>> Driver: enhance MSC version compatibility
>>>>>>>>
>>>>>>>> The version information for Visual Studio is
>>>>>>>> spread
>>>>>>>> over
>>>>>>>> multiple variables.
>>>>>>>> The newer Windows SDK has started making use of
>>>>>>>> some
>>>>>>>> of the
>>>>>>>> extended versioning
>>>>>>>> variables that were previously undefined. Enhance
>>>>>>>> our
>>>>>>>> compatibility definitions
>>>>>>>> for these cases.
>>>>>>>>
>>>>>>>> _MSC_VER is defined to be the Major * 100 + Minor.
>>>>>>>> _MSC_FULL_VER is defined to
>>>>>>>> be Major * 10000000 + Minor * 100000 + Build. And
>>>>>>>> _MSC_BUILD
>>>>>>>> is the build
>>>>>>>> revision of the compiler.
>>>>>>>>
>>>>>>>> Extend the -fmsc-version option in a compatible
>>>>>>>> manner. If
>>>>>>>> the value is the
>>>>>>>> previous form of MMmm, then we assume that the
>>>>>>>> build
>>>>>>>> number is
>>>>>>>> 0. Otherwise, a
>>>>>>>> specific build number may be passed by using the
>>>>>>>> form
>>>>>>>> MMmmbbbbb.
>>>>>>>>
>>>>>>>>
>>>>>>>> We shouldn't overload the semantics of compiler
>>>>>>>> options
>>>>>>>> like this,
>>>>>>>> because doing so:
>>>>>>>>
>>>>>>>>
>>>>>>>> I did this based on review feedback.
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Saleem,
>>>>>>>>
>>>>>>>> I read the review thread and don't see any mention of
>>>>>>>> canonicalisation issues, or the bug where 17.0 and 17 are
>>>>>>>> mishandled. The review was mostly about coding style.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140616/108136.html
>>>>>>>>
>>>>>>>>
>>>>>>>> I have a slight preference for it, but its not a strong
>>>>>>>> opinion. I would rather Reid and Aaron chime in and we
>>>>>>>> decide
>>>>>>>> on the path we wish to take before I spend effort to try
>>>>>>>> to
>>>>>>>> change the behaviour added here.
>>>>>>>>
>>>>>>>>
>>>>>>>> I wrote a detailed point-by-point review about how this
>>>>>>>> doesn't
>>>>>>>> fit in with the design of our driver or expected usage --
>>>>>>>> could
>>>>>>>> you be more specific?
>>>>>>>>
>>>>>>>>
>>>>>>>> I was referring to the dot-separated value.
>>>>>>>
>>>>>>> Accepting a dotted version is fine. Overloading the existing
>>>>>>> -fmcs-version=
>>>>>>> option is the problem..
>>>>>>
>>>>>> Given that this is meant to be backwards compatible, I'm not certain I
>>>>>> understand what the problem actually is. If the option isn't backwards
>>>>>> compatible, that's simply a bug we should fix.
>>>>>>
>>>>>>>> * Makes configure-time feature detection harder ("but
>>>>>>>> the
>>>>>>>> option
>>>>>>>> exists, why doesn't it accept my input?")
>>>>>>>>
>>>>>>>>
>>>>>>>> This seems like it is a pre-existing problem, not something specific
>>>>>>>> to
>>>>>>>> this change.
>>>>>>>
>>>>>>> Nope, definitely specific to this change.
>>>>>>
>>>>>> Can you elaborate?
>>>>>>
>>>>>>>> * Confuses argument canonicalization routines in
>>>>>>>> external
>>>>>>>> tools
>>>>>>>> like ccache, potentially requiring special handling to
>>>>>>>> be
>>>>>>>> duplicated in each external project.
>>>>>>>>
>>>>>>>>
>>>>>>>> Can you provide some concrete example of this please? Im afraid I
>>>>>>>> dont
>>>>>>>> see how that would happen.
>>>>>>>
>>>>>>> External tools need to parse and grok driver arguments. Supporting
>>>>>>> two
>>>>>>> syntaxes in a single option requires custom handling to be developed
>>>>>>> in
>>>>>>> each
>>>>>>> tool -- there are very few other options like yours in the clang
>>>>>>> driver.
>>>>>>> Where they exist, it's done out of necessity to support GCC bugs.
>>>>>>>
>>>>>>> Your feature could easily be implemented as a second flag at no cost,
>>>>>>> and
>>>>>>> that seems to be the obvious way to go. Is there a problem with
>>>>>>> actually
>>>>>>> doing that?
>>>>>>
>>>>>> Then users (whose experience I care about *far* more than external
>>>>>> tools vendors) have two options which serve the same purpose, and
>>>>>> creates some confusion as to which one should be used, what situations
>>>>>> they should use them in, and why. Having a single option presents a
>>>>>> better user experience, IMO.
>>>>>
>>>>> This is interesting. To me, it's as clear as day that accepting two
>>>>> incompatible version formats in a single option flag is bad for users
>>>>> and
>>>>> tools. It's also unprecedented -- we've not done this before with any
>>>>> driver
>>>>> option. There are many options that have multiple forms for different
>>>>> inputs
>>>>> syntax.
>>>>>
>>>>> Why is this flag so special that we need to break all the rules and
>>>>> introduce command-line parsing disambiguations?
>>>>>
>>>>> It's not at all obvious to a user that '17.0' or '17.0.0' defines
>>>>> macros
>>>>> as version 17 while '17' defines macros as version 0.0.0.17.
>>>>> Predicating on
>>>>> presence of a '.' is going to look like a bug to any user who
>>>>> encounters it.
>>>>
>>>> This seems truly insincere. Do you honestly expect developers to claim,
>>>> version 17 is 0.0.17
>>>
>>>
>>> Hi Saleem,
>>>
>>> That's what your patch does right now, which is the bug I'm reporting.
>>>
>>> You missed the base case in your code so when the user specifies 17 it
>>> turns
>>> into 0.0.0.17 with your patch.
>>
>> I think we're all agreed that behavior is simply a bug that needs to
>> be resolved. 17 should become 17.0.
>
>
> Yes, and major[.minor[.patch[.release]]]] will conflict with the traditional
> integer forms conflict in that case.
>
> I don't see any way to support both integral forms cleanly with a single
> option.
The backwards compatible approach is to look at the value which comes
in. It could only encode a specific range of values (MMmm), which
means for MM values, 99 was the max. I'm perfectly comfortable saying
any value that comes in < 100 is simply a major version.
~Aaron
More information about the cfe-commits
mailing list