We've started discussing a cross-vendor __has_attribute mechanism in the C++ features SG, so I'd like us to hold off on any action here until that discussion reaches consensus.
On Thu, Mar 27, 2014 at 11:48 AM, Aaron Ballman <[email protected]>wrote: > On Sun, Mar 9, 2014 at 8:46 PM, Richard Smith <[email protected]> > wrote: > > On Sat, Mar 8, 2014 at 2:51 PM, Aaron Ballman <[email protected]> > > wrote: > >> > >> 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? > > > > > > Sorry it's taken me a while to circle back to this. > > > > It seems like the biggest point of contention is how this new feature > > interacts with old Clang installations (or other compilers) that lack the > > feature, and the discoverability of the relevant __has_feature check. > > > > Right now we recommend something like: > > > > #ifndef __has_attribute > > #define __has_attribute(x) 0 > > #endif > > > > That'll work fine for non-Clang compilers and for new Clang, but not for > old > > Clang. If we use __has_attribute as the name of the new thing, old > versions > > of Clang will say: > > > > error: builtin feature check macro requires a parenthesized identifier > > > > ... which is pretty clear, even though it doesn't point to the relevant > > __has_feature check. If we use __has_attribute_syntax, old versions of > Clang > > will diagnose the attribute *within* the parentheses, rather than the > use of > > __has_attribute_syntax itself. So it seems that keeping the same name > leads > > us to recover somewhat better. > > > > So I'm weakly in favor of extending __has_attribute rather than > introducing > > a new name for this functionality -- especially if we can also add a > > -Wdeprecated warning for the old form of __has_attribute, suggesting a > > migration to the new form. > > I kind of dropped the ball a bit on following up here, sorry about > that. But it turns out I need this functionality in the parser as > well, which reminded me to come back to this. > > This patch has been rebased against trunk, but it differs in terms of > layering than the previous version. The previous version had a > HasAttribute static helper function in PPMacroExpansion.cpp. This > version exposes HasAttribute from the basic layer, which is a more > sensible of a place for it given that I need to use it in the parser > (and it's not really preprocessor-specific functionality). Note that > the preprocessor parsing functionality remains in the preprocessor -- > the only thing that moved was the code checking whether the attribute > is actually supported. > > Given Richard's tie-break, I think this feature is ready to commit > (barring feedback, of course). > > ~Aaron >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
