On 23/06/2014 20:48, Aaron Ballman wrote:
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.
You're suggesting this?
if (version < 100)
version *= 100;
It doesn't make sense to me.
Why parse an integer driver argument completely differently depending on
whether it's "very big or very small" when there's no burden of
compatibility that forces us to push the two totally value kinds into a
single flag?
There's a really elegant way to do this that's also very neatly
documented, versioned and more discoverable, while requiring no
disambiguation:
-fmsc-version=MMmmbbbbb
-fmsc-full-version=major[.minor[.patch[.release]]]]
What's more, those types are both represented by standard masks in at
least XCode and MSVC and probably other tools too. That's really cool to
me and strongly indicates we want two option flags here.
I'd be interested to hear what others think.
Alp.
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
