<div dir="ltr">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.</div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Mar 27, 2014 at 11:48 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sun, Mar 9, 2014 at 8:46 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Sat, Mar 8, 2014 at 2:51 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Feb 25, 2014 at 10:33 AM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:<br>
>> ><br>
>> > On 25/02/2014 15:13, Alp Toker wrote:<br>
>> ><br>
>> ><br>
>> > On 25/02/2014 13:45, Aaron Ballman wrote:<br>
>> ><br>
>> > On Mon, Feb 24, 2014 at 6:46 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:<br>
>> ><br>
>> > On 24/02/2014 16:19, David Blaikie wrote:<br>
>> ><br>
>> ><br>
>> > On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a><br>
>> > <mailto:<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>>> wrote:<br>
>> ><br>
>> >     Hi Aaron,<br>
>> ><br>
>> >     As a fix for PR15853 this is looking OK and almost there.<br>
>> ><br>
>> >     It's quirky to put a synthesis of the attribute parse rules in the<br>
>> >     preprocessor but your approach meets needs that the other<br>
>> >     solutions don't so it's cool.<br>
>> ><br>
>> >     However the addition of new semantics to the existing<br>
>> >     __has_feature() macro is problematic given that even ToT clang<br>
>> >     will error out the preprocessor whenever new-style arguments are<br>
>> >     encountered. That's bad news for a feature check macro that needs<br>
>> >     to work on a range of clang versions.<br>
>> ><br>
>> >     I know you've since added __has_feature(__has_attribute_syntax) to<br>
>> >     work around the problem but it's not discoverable and I suspect<br>
>> >     the feature check won't get used correctly in the wild.<br>
>> ><br>
>> >     So how about just naming the new check __has_attribute_syntax()?<br>
>> ><br>
>> ><br>
>> > Why would this be more discoverable than<br>
>> > __has_feature(__has_attribute_syntax)?<br>
>> ><br>
>> > If a user writes __has_attribute_syntax() and it doesn't work on some<br>
>> > other<br>
>> > compiler, the natural thing to do is to check and conditionalize on<br>
>> > whether<br>
>> > the macro exists with ifdef.<br>
>> ><br>
>> ><br>
>> > #ifndef __has_attribute_syntax<br>
>> > #define __has_attribute_syntax(...)<br>
>> > #endif<br>
>> ><br>
>> > That's how the existing feature check macros work and users have been<br>
>> > successfully conditionalizing the macros in this way.<br>
>> ><br>
>> > The trouble is that __has_attribute() already exists so changing the<br>
>> > semantics will break current versions of clang:<br>
>> ><br>
>> > #ifndef __has_attribute<br>
>> > #define __has_attribute([[deprecated]]) // no good, preprocessor error<br>
>> > on<br>
>> > clang ToT<br>
>> > #endif<br>
>> ><br>
>> > This same problem exists with __has_attribute_syntax.<br>
>> ><br>
>> > It's non-obvious for the user to then make the leap that this<br>
>> > __has_attribute() check needs to be conditionalized against a second<br>
>> > level<br>
>> > __has_feature(__has_attribute_syntax) because the user has to learn two<br>
>> > other features -- '__has_feature' and '__has_attribute_syntax' which<br>
>> > have<br>
>> > little connection to '__has_attribute'  in order to safely use<br>
>> > '__has_attribute' with the new semantics.<br>
>> ><br>
>> > Adding the functionality with a new name makes it usable without<br>
>> > resorting<br>
>> > to the compiler documentation.<br>
>> ><br>
>> > I disagree -- you still have to refer to the documentation because you<br>
>> > have no idea __has_attribute_syntax exists in the first place (esp<br>
>> > when __has_attribute has been around for a while).<br>
>> ><br>
>> > Richard had pointed out a while back that<br>
>> > __has_feature(__has_attribute_syntax) is morally equivalent to using<br>
>> > __has_attribute_syntax as a new spelling for __has_attribute, and I<br>
>> > agree. I would rather use __has_attribute as<br>
>> ><br>
>> ><br>
>> ><br>
>> > 1) it's already an<br>
>> > established API that we can extend,<br>
>> ><br>
>> ><br>
>> > Well, my take on this is that it's an established API that wasn't<br>
>> > designed<br>
>> > to be extensible :-)<br>
>> ><br>
>> > 2) it's a less awkward spelling<br>
>> ><br>
>> ><br>
>> > Granted, but that needs to be weighed up against the cost to users of<br>
>> > breaking backward compatibility.<br>
>> ><br>
>> > for the functionality, and 3) It's one less API for users to have to<br>
>> > keep in their brain.<br>
>> ><br>
>> ><br>
>> > Not really, it's three things to learn (bold emphasis on the points that<br>
>> > need to be learnt below):<br>
>> ><br>
>> > #if defined(__has_feature) && __has_feature(__has_attribute_syntax) &&<br>
>> > __has_attribute([[deprecated]])<br>
>> ><br>
>> > Instead of one thing to learn:<br>
>> ><br>
>> > #if defined(__has_attribute_syntax) &&<br>
>> > __has_attribute_syntax([[deprecated]])<br>
>> ><br>
>> > I prefer the latter because it matches the prescribed usage for all the<br>
>> > other feature detection macros:<br>
>> ><br>
>> ><br>
>> > #ifndef __has_builtin         // Optional of course.<br>
>> >   #define __has_builtin(x) 0  // Compatibility with non-clang compilers.<br>
>> > #endif<br>
>> ><br>
>> ><br>
>> > #ifndef __has_feature         // Optional of course.<br>
>> >   #define __has_feature(x) 0  // Compatibility with non-clang compilers.<br>
>> > #endif<br>
>> ><br>
>> > #if defined(__has_include)<br>
>> > #if __has_include("myinclude.h")<br>
>> > # include "myinclude.h"<br>
>> > #endif<br>
>> > #endif<br>
>> ><br>
>> > etc.<br>
>> ><br>
>> > Is the old name really that precious that we'd be willing to make<br>
>> > __has_attribute() a special case that's inconsistent with the other<br>
>> > feature<br>
>> > macros?<br>
>> ><br>
>> > Also, remember that the "older versions of clang"<br>
>> > problem will go away with time, but the semi-duplicated syntax is<br>
>> > something we'd have forever.<br>
>> ><br>
>> ><br>
>> > Changing the semantics means pain for our users until those compilers go<br>
>> > away, and compilers have a tendency to stick around :-/<br>
>> ><br>
>> > Also we've been trying to clear up __has_feature()  (see my proposal on<br>
>> > C++<br>
>> > type trait detection) so it grates that we'd be adding another<br>
>> > non-language-standard check while at the same time trying to deprecate<br>
>> > the<br>
>> > existing ones:<br>
>> ><br>
>> > "__has_feature evaluates to 1 if the feature is both supported by Clang<br>
>> > and<br>
>> > standardized in the current language standard or 0 if not. For backwards<br>
>> > compatibility reasons, __has_feature can also be used to test for<br>
>> > support<br>
>> > for non-standardized features, i.e. features not prefixed c_, cxx_<br>
>> > orobjc_."<br>
>> ><br>
>> ><br>
>> > And I forgot to mention, changing the semantics of __has_attribute()<br>
>> > then<br>
>> > recommending users guard it with __has_feature(__has_attribute_syntax)<br>
>> > makes<br>
>> > it difficult for other vendors to implement the proposed feature<br>
>> > independently without also supporting a clang-style __has_feature()<br>
>> > detection macro. This is secondary but still worth keeping in mind.<br>
>> ><br>
>> ><br>
>> ><br>
>> > I don't mean to be contrary but I did want to write out my justification<br>
>> > in<br>
>> > full here from a language design perspective. If you know all this and<br>
>> > want<br>
>> > to go ahead, it's your module :-)<br>
>> ><br>
>> ><br>
>> > It may be in practice that the points raised don't matter much and I'd<br>
>> > like<br>
>> > to avoid confirmation bias -- perhaps Richard can tie-break?<br>
>><br>
>> Ping?<br>
><br>
><br>
> Sorry it's taken me a while to circle back to this.<br>
><br>
> It seems like the biggest point of contention is how this new feature<br>
> interacts with old Clang installations (or other compilers) that lack the<br>
> feature, and the discoverability of the relevant __has_feature check.<br>
><br>
> Right now we recommend something like:<br>
><br>
> #ifndef __has_attribute<br>
> #define __has_attribute(x) 0<br>
> #endif<br>
><br>
> That'll work fine for non-Clang compilers and for new Clang, but not for old<br>
> Clang. If we use __has_attribute as the name of the new thing, old versions<br>
> of Clang will say:<br>
><br>
> error: builtin feature check macro requires a parenthesized identifier<br>
><br>
> ... which is pretty clear, even though it doesn't point to the relevant<br>
> __has_feature check. If we use __has_attribute_syntax, old versions of Clang<br>
> will diagnose the attribute *within* the parentheses, rather than the use of<br>
> __has_attribute_syntax itself. So it seems that keeping the same name leads<br>
> us to recover somewhat better.<br>
><br>
> So I'm weakly in favor of extending __has_attribute rather than introducing<br>
> a new name for this functionality -- especially if we can also add a<br>
> -Wdeprecated warning for the old form of __has_attribute, suggesting a<br>
> migration to the new form.<br>
<br>
</div></div>I kind of dropped the ball a bit on following up here, sorry about<br>
that. But it turns out I need this functionality in the parser as<br>
well, which reminded me to come back to this.<br>
<br>
This patch has been rebased against trunk, but it differs in terms of<br>
layering than the previous version. The previous version had a<br>
HasAttribute static helper function in PPMacroExpansion.cpp. This<br>
version exposes HasAttribute from the basic layer, which is a more<br>
sensible of a place for it given that I need to use it in the parser<br>
(and it's not really preprocessor-specific functionality). Note that<br>
the preprocessor parsing functionality remains in the preprocessor --<br>
the only thing that moved was the code checking whether the attribute<br>
is actually supported.<br>
<br>
Given Richard's tie-break, I think this feature is ready to commit<br>
(barring feedback, of course).<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div>