<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Mar 8, 2014 at 2:51 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">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 other<br>
> compiler, the natural thing to do is to check and conditionalize on 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 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 level<br>
> __has_feature(__has_attribute_syntax) because the user has to learn two<br>
> other features -- '__has_feature' and '__has_attribute_syntax' which 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 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 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 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 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 the<br>
> existing ones:<br>
><br>
> "__has_feature evaluates to 1 if the feature is both supported by Clang 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 support<br>
> for non-standardized features, i.e. features not prefixed c_, cxx_ orobjc_."<br>
><br>
><br>
> And I forgot to mention, changing the semantics of __has_attribute() then<br>
> recommending users guard it with __has_feature(__has_attribute_syntax) 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 in<br>
> full here from a language design perspective. If you know all this and 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 like<br>
> to avoid confirmation bias -- perhaps Richard can tie-break?<br>
<br>
</div></div>Ping?</blockquote><div><br></div><div>Sorry it's taken me a while to circle back to this.</div><div><br></div><div>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.</div>
<div><br></div><div>Right now we recommend something like:</div><div><br></div><div>#ifndef __has_attribute</div><div>#define __has_attribute(x) 0</div><div>#endif</div><div><br></div><div>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:<br>
<br></div><div>error: builtin feature check macro requires a parenthesized identifier</div><div><br></div><div>... 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.</div>
<div><br></div><div>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.</div>
</div></div></div>