r211420 - Driver: enhance MSC version compatibility
Aaron Ballman
aaron at aaronballman.com
Mon Jun 23 09:54:06 PDT 2014
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.
~Aaron
More information about the cfe-commits
mailing list