On Mon, Jun 23, 2014 at 1:36 PM, Alp Toker <[email protected]> wrote: > > On 23/06/2014 19:54, Aaron Ballman wrote: >> >> 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. > > > 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
