<div dir="ltr"><div>[Adding back cfe-dev]</div><div><br></div>On Fri, Dec 6, 2013 at 10:42 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>

<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">So my plan for making __has_attribute target-aware is to basically get<br>


rid of TargetAttributeSema.cpp entirely. I'll update Attr.td to<br>
specify target information for the attributes, and SemaDeclAttr.cpp<br>
will dispatch target-specific attribute logic the same way it does<br>
target-non-specific attributes. Just that the common attribute handler<br>
will filter out target-specific attributes that don't apply to the<br>
current target. Then I can update HasAttribute's table-generated code<br>
to pay attention to the current target.<br></blockquote><div><br></div><div>That sounds great to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


Also of note: HasAttribute currently does the wrong thing with<br>
underscores. It'll happily tell you __has_attribute(__dllexport__)<br>
exists when it shouldn't. I'll figure something out for that as well;<br>
probably by consolidating it with AttributeList::getKind in some way.<br></blockquote><div><br></div><div>See bug 15853: in the shorter term, we should probably not be considering spellings other than GNU spellings in __has_attribute anyway. As Alp points out there, we will probably need a different approach there to support other attribute syntaxes.</div>
<div><br></div><div>Long-term, I'd like to completely separate building of an *Attr object from attaching it to things. When that's done, we could support a general __is_valid_attribute check by parsing and building a *Attr object, and seeing if a diagnostic is produced; that'd allow things like __is_valid_attribute(__attribute__((printf(some_new_syntax, 1, 2)))) to check for a specific flavour of an attribute.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Thanks for the feedback!<br>
<span><font color="#888888"><br>
~Aaron<br>
</font></span><div><div><br>
On Fri, Dec 6, 2013 at 1:28 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> On Fri, Dec 6, 2013 at 9:33 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Currently, the __has_attribute feature merely looks to see whether the<br>
>> given attribute name is known. It does not care whether the attribute<br>
>> is known for a particular target. So, for instance, the mips16<br>
>> attribute is only known to the Mips target, but<br>
>> __has_attribute(mips16) will return 1 for x86. There is at least one<br>
>> case in the wild (with chrome tsan) where the assumption was that<br>
>> __has_attribute was target-aware.<br>
>><br>
>> After discussing this on IRC, the general consensus was that the<br>
>> current behavior is less than desirable because it requires the author<br>
>> to write #if target && __has_attribute which kind of defeats the<br>
>> purpose of __has_attribute in the first place.<br>
>><br>
>> Work can be done to make __has_attribute target-aware, however, it<br>
>> would then make __has_attribute behave differently than __has_builtin<br>
>> (for example), so it may not be a desirable direction. It's also a<br>
>> functional change to a shipping feature. So I am proceeding with<br>
>> caution.<br>
>><br>
>> As I see it, there are three options:<br>
>><br>
>> 1) Change __has_attribute to be target-aware, update the documentation<br>
>> regarding the new functionality.<br>
>> 2) Leave the feature as-is, and update the documentation to be clear<br>
>> that __has_attribute is not target-aware.<br>
>> 3) Leave everything as it stands (including documentation) so we can<br>
>> change our minds in the future. I don't much care for this option.<br>
>><br>
>> I am wondering what the proper direction to proceed is.<br>
><br>
><br>
> The general principle of __has_* is that they tell you about the current<br>
> state of the compiler (hence __has_feature and __has_extension will produce<br>
> different answers based on the current language mode). We do this even in<br>
> crazy cases like:<br>
><br>
>   int a = __has_builtin(printf); // a == 1<br>
>   extern "C" void printf();<br>
>   int b = __has_builtin(printf); // b == 0<br>
><br>
> I'm not sure what you mean about __has_builtin, by the way: that already<br>
> does the right thing for target-specific attributes.<br>
><br>
>   int a = __has_builtin(__builtin_ia32_femms); // a == 1 on x86 and 0<br>
> elsewhere<br>
><br>
> So I think the right thing is:<br>
><br>
>   4) Fix __has_attribute to be target-aware, leave the documentation alone.<br>
><br>
> Essentially, I think this is just a bug: __has_attribute was giving a false<br>
> negative for supported attributes in some cases.<br>
</div></div></blockquote></div><br></div></div>