[cfe-dev] [PATCH] New syntax and functionality for __has_attribute

[Aaron Ballman]

On Mon, Feb 24, 2014 at 11:19 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <alp at nuanti.com> 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)?

I don't feel it would be.

As I see it, there are two models we could use: __has_attribute with
syntax support, or __has_attribute/__has_attribute_syntax. However, I
don't see a benefit to __has_attribute_syntax for compatibility (note,
the attribute used isn't important):

1) Not backwards compatible, regardless of spelling for the feature-test macro.
#if __has_attribute([[clang::fallthrough]])
#endif

2) Backwards compatible 1:
#if __has_feature(__has_attribute_syntax) &&
    __has_attribute([[clang::fallthrough]])
#endif

3) Backwards compatible 2:
#if defined(__has_attribute_syntax) &&
    __has_attribute_syntax([[clang::fallthrough]])
#endif

4) Backwards compatible because no syntax is used:
#if __has_attribute(hot)
#endif

#2 and #3 achieve the same goal, in basically the same way. #1 ignores
the problem, and #4 is never a problem anyway. I prefer sticking with
the __has_attribute spelling because it's already known to users, and
it means there's one API to check whether an attribute is supported
instead of two APIs. But perhaps Alp has a usage in mind that I'm not
envisioning.

~Aaron