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

[Aaron Ballman]

On Mon, Feb 24, 2014 at 6:46 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 24/02/2014 16:19, David Blaikie wrote:
>
>>
>>
>>
>> On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <alp at nuanti.com
>> <mailto: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)?
>
>
> 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, 2) it's a less awkward spelling
for the functionality, and 3) It's one less API for users to have to
keep in their brain. 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.

~Aaron