On Tue, Feb 25, 2014 at 10:33 AM, Alp Toker <[email protected]> wrote: > > On 25/02/2014 15:13, Alp Toker wrote: > > > On 25/02/2014 13:45, Aaron Ballman wrote: > > On Mon, Feb 24, 2014 at 6:46 PM, Alp Toker <[email protected]> wrote: > > On 24/02/2014 16:19, David Blaikie wrote: > > > On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <[email protected] > <mailto:[email protected]>> wrote: > > Hi Aaron, > > As a fix for PR15853 this is looking OK and almost there. > > It's quirky to put a synthesis of the attribute parse rules in the > preprocessor but your approach meets needs that the other > solutions don't so it's cool. > > However the addition of new semantics to the existing > __has_feature() macro is problematic given that even ToT clang > will error out the preprocessor whenever new-style arguments are > encountered. That's bad news for a feature check macro that needs > to work on a range of clang versions. > > I know you've since added __has_feature(__has_attribute_syntax) to > work around the problem but it's not discoverable and I suspect > the feature check won't get used correctly in the wild. > > So how about just naming the new check __has_attribute_syntax()? > > > Why would this be more discoverable than > __has_feature(__has_attribute_syntax)? > > If a user writes __has_attribute_syntax() and it doesn't work on some other > compiler, the natural thing to do is to check and conditionalize on whether > the macro exists with ifdef. > > > #ifndef __has_attribute_syntax > #define __has_attribute_syntax(...) > #endif > > That's how the existing feature check macros work and users have been > successfully conditionalizing the macros in this way. > > The trouble is that __has_attribute() already exists so changing the > semantics will break current versions of clang: > > #ifndef __has_attribute > #define __has_attribute([[deprecated]]) // no good, preprocessor error on > clang ToT > #endif > > This same problem exists with __has_attribute_syntax. > > It's non-obvious for the user to then make the leap that this > __has_attribute() check needs to be conditionalized against a second level > __has_feature(__has_attribute_syntax) because the user has to learn two > other features -- '__has_feature' and '__has_attribute_syntax' which have > little connection to '__has_attribute' in order to safely use > '__has_attribute' with the new semantics. > > Adding the functionality with a new name makes it usable without resorting > to the compiler documentation. > > I disagree -- you still have to refer to the documentation because you > have no idea __has_attribute_syntax exists in the first place (esp > when __has_attribute has been around for a while). > > Richard had pointed out a while back that > __has_feature(__has_attribute_syntax) is morally equivalent to using > __has_attribute_syntax as a new spelling for __has_attribute, and I > agree. I would rather use __has_attribute as > > > > 1) it's already an > established API that we can extend, > > > Well, my take on this is that it's an established API that wasn't designed > to be extensible :-) > > 2) it's a less awkward spelling > > > Granted, but that needs to be weighed up against the cost to users of > breaking backward compatibility. > > for the functionality, and 3) It's one less API for users to have to > keep in their brain. > > > Not really, it's three things to learn (bold emphasis on the points that > need to be learnt below): > > #if defined(__has_feature) && __has_feature(__has_attribute_syntax) && > __has_attribute([[deprecated]]) > > Instead of one thing to learn: > > #if defined(__has_attribute_syntax) && > __has_attribute_syntax([[deprecated]]) > > I prefer the latter because it matches the prescribed usage for all the > other feature detection macros: > > > #ifndef __has_builtin // Optional of course. > #define __has_builtin(x) 0 // Compatibility with non-clang compilers. > #endif > > > #ifndef __has_feature // Optional of course. > #define __has_feature(x) 0 // Compatibility with non-clang compilers. > #endif > > #if defined(__has_include) > #if __has_include("myinclude.h") > # include "myinclude.h" > #endif > #endif > > etc. > > Is the old name really that precious that we'd be willing to make > __has_attribute() a special case that's inconsistent with the other feature > macros? > > Also, remember that the "older versions of clang" > problem will go away with time, but the semi-duplicated syntax is > something we'd have forever. > > > Changing the semantics means pain for our users until those compilers go > away, and compilers have a tendency to stick around :-/ > > Also we've been trying to clear up __has_feature() (see my proposal on C++ > type trait detection) so it grates that we'd be adding another > non-language-standard check while at the same time trying to deprecate the > existing ones: > > "__has_feature evaluates to 1 if the feature is both supported by Clang and > standardized in the current language standard or 0 if not. For backwards > compatibility reasons, __has_feature can also be used to test for support > for non-standardized features, i.e. features not prefixed c_, cxx_ orobjc_." > > > And I forgot to mention, changing the semantics of __has_attribute() then > recommending users guard it with __has_feature(__has_attribute_syntax) makes > it difficult for other vendors to implement the proposed feature > independently without also supporting a clang-style __has_feature() > detection macro. This is secondary but still worth keeping in mind. > > > > I don't mean to be contrary but I did want to write out my justification in > full here from a language design perspective. If you know all this and want > to go ahead, it's your module :-) > > > It may be in practice that the points raised don't matter much and I'd like > to avoid confirmation bias -- perhaps Richard can tie-break?
Ping? ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
