r211420 - Driver: enhance MSC version compatibility
Alp Toker
alp at nuanti.com
Mon Jun 23 10:36:48 PDT 2014
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.
Alp.
> ~Aaron
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list