On Mon, Jun 23, 2014 at 12:50 PM, Alp Toker <[email protected]> wrote: > > On 23/06/2014 19:27, Saleem Abdulrasool wrote: >> >> On Jun 23, 2014, at 6:38 AM, Alp Toker <[email protected]> wrote: >> >>> On 23/06/2014 15:02, Aaron Ballman wrote: >>>> >>>> On Sun, Jun 22, 2014 at 10:09 PM, Alp Toker <[email protected]> wrote: >>>>> >>>>> On 23/06/2014 04:30, Saleem Abdulrasool wrote: >>>>>> >>>>>> On Sun, Jun 22, 2014 at 4:15 PM, Alp Toker <[email protected] >>>>>> <mailto:[email protected]>> wrote: >>>>>> >>>>>> >>>>>> On 23/06/2014 01:53, Saleem Abdulrasool wrote: >>>>>> >>>>>> On Sat, Jun 21, 2014 at 11:32 AM, Alp Toker <[email protected] >>>>>> <mailto:[email protected]> <mailto:[email protected] >>>>>> >>>>>> <mailto:[email protected]>>> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
