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

[Alp Toker]

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()?

That way the widely-recognised feature detection fallback pattern will 
just work and you can drop the  __has_feature workaround entirely. Users 
will be able to use the familiar:

#ifndef __has_attribute_syntax
#define __has_attribute_syntax(...)
#endif

Doing this also means we can remove the old, ambiguous identifier-only 
form of the check from the new version of the check. With this change to 
your proposed patch we can ensure safe and compatible usage which is 
important with vendor extensions like this.

Alp.


On 23/02/2014 16:24, Aaron Ballman wrote:
> Ping
>
> ~Aaron
>
> On Wed, Feb 19, 2014 at 1:05 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> This is the latest version of this patch -- with some additional
>> features as requested. Specifically, this is using the existing
>> __has_attribute syntax in an extended style, and implements a
>> feature-test macro (__has_feature(__has_attribute_syntax)) which
>> allows you to determine whether the extended syntax is supported or
>> not. It adds some additional documentation explaining this.
>>
>> ~Aaron
>>
>> On Thu, Jan 23, 2014 at 10:23 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> Before I start in on this again, I wanted to make sure that there was
>>> a consensus that this functionality was desirable. I believe the
>>> answer (based on some conversations in IRC and here on the lists) was
>>> tentatively "yes."
>>>
>>> As far as I can tell, the work left to be done on this is to add a
>>> feature test for __has_attribute_syntax, write the documentation for
>>> it and that's about it? The syntax-based form is the preferable
>>> nomenclature because it leaves the door open for testing parameters at
>>> some point in the future, and with the __has_attribute_syntax feature
>>> test, it is both backwards and forwards compatible.
>>>
>>> ~Aaron
>>>
>>> On Mon, Jan 13, 2014 at 8:49 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>> On Mon, Jan 13, 2014 at 5:44 PM, Aaron Ballman <aaron at aaronballman.com>
>>>> wrote:
>>>>> On Mon, Jan 13, 2014 at 8:39 PM, Richard Smith <richard at metafoo.co.uk>
>>>>> wrote:
>>>>>> On Mon, Jan 13, 2014 at 5:31 PM, Aaron Ballman <aaron at aaronballman.com>
>>>>>> wrote:
>>>>>>> On Mon, Jan 13, 2014 at 8:20 PM, Richard Smith <richard at metafoo.co.uk>
>>>>>>> wrote:
>>>>>>>> On Sun, Jan 12, 2014 at 4:49 PM, Aaron Ballman
>>>>>>>> <aaron at aaronballman.com>
>>>>>>>> wrote:
>>>>>>>>> On Sun, Jan 12, 2014 at 7:38 PM, Sean Silva <silvas at purdue.edu>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Sun, Jan 12, 2014 at 6:53 PM, Aaron Ballman
>>>>>>>>>> <aaron at aaronballman.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>> That's good news -- thanks for confirming.
>>>>>>>>>>>>
>>>>>>>>>>>> The feature detection macro itself will still need to have a
>>>>>>>>>>>> different
>>>>>>>>>>>> name
>>>>>>>>>>>> (or some other mechanism) so it can be used compatibly with
>>>>>>>>>>>> existing
>>>>>>>>>>>> clang
>>>>>>>>>>>> deployments, because _has_attribute() currently emits a parse
>>>>>>>>>>>> error
>>>>>>>>>>>> instead
>>>>>>>>>>>> of gracefully returning 0 when passed the new argument syntax:
>>>>>>>>>>>>
>>>>>>>>>>>> tmp/attr2.cpp:1:5: error: builtin feature check macro requires
>>>>>>>>>>>> a
>>>>>>>>>>>> parenthesized identifier
>>>>>>>>>>>> #if __has_attribute(__attribute__((weakref)))
>>>>>>>>>>>>      ^
>>>>>>>>>>> Good catch. Unfortunately, __has_attribute is really the best
>>>>>>>>>>> identifier for the macro, so I am loathe to let it go.
>>>>>>>>>>>
>>>>>>>>>>> Due to the current design of __has_attribute, we can't get away
>>>>>>>>>>> with
>>>>>>>>>>> "
>>>>>>>>>>> magic" by expanding the non-function-like form into a value that
>>>>>>>>>>> could
>>>>>>>>>>> be tested. So we would really have to pick a new name if we are
>>>>>>>>>>> worried about this.
>>>>>>>>>>>
>>>>>>>>>>> Suggestions on the name are welcome.
>>>>>>>>>>
>>>>>>>>>> Ok, I'll bite:
>>>>>>>>>>
>>>>>>>>>> __has_attribute_written_as([[foo]])
>>>>>>>>>> __has_attribute_syntax([[foo]])
>>>>>>>>>> __has_attribute_spelling([[foo]])
>>>>>>>>> I kind of like __has_attribute_syntax, truth be told.
>>>>>>>>
>>>>>>>> Have we given up on using the name __has_attribute too soon? Users of
>>>>>>>> the
>>>>>>>> new syntax could write:
>>>>>>>>
>>>>>>>> // Probably already in project's porting header
>>>>>>>> #ifndef __has_feature
>>>>>>>> #define __has_feature(x) 0
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> #if __has_feature(__has_attribute_syntax)
>>>>>>>> #define MY_HAS_ATTRIBUTE(...) __has_attribute(__VA_ARGS__)
>>>>>>>> #else
>>>>>>>> #define MY_HAS_ATTRIBUTE(...) 0
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> If it's given a different name, they instead would write something
>>>>>>>> like:
>>>>>>>>
>>>>>>>> #ifdef __has_attribute_syntax
>>>>>>>> #define MY_HAS_ATTRIBUTE(...) __has_attribute_syntax(__VA_ARGS__)
>>>>>>>> #else
>>>>>>>> #define MY_HAS_ATTRIBUTE(...) 0
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> So I don't think the change-in-syntax argument holds water.
>>>>>>> So are you proposing that we would have a different name for the
>>>>>>> purposes of the __has_feature macro? Eg)
>>>>>>> __has_feature(__has_attribute_syntax) is 1 for the proposed
>>>>>>> functionality, and 0 otherwise?
>>>>>>
>>>>>> It's a possibility. It could be that a new name is a better approach,
>>>>>> but
>>>>>> both directions seem to be feasible.
>>>>> I'll ponder; I rather like keeping the existing name.
>>>>
>>>> By the same argument, it's possible to add extra arguments to
>>>> __has_attribute, if we have a __has_feature check for the new syntax.
>>>>
>>>>>>>> Also, supporting arguments in the attributes is useful in some cases
>>>>>>>> --
>>>>>>>> it's
>>>>>>>> not true that they don't make sense in a feature-checking facility.
>>>>>>>> For
>>>>>>>> instance:
>>>>>>>>
>>>>>>>>    __has_attribute( __attribute__((format)) )
>>>>>>>>
>>>>>>>> ... doesn't tell me whether __attribute__((format, gnu_scanf, 1, 2)
>>>>>>>> will
>>>>>>>> work (and I'd expect that the format attribute will gain additional
>>>>>>>> archetypes in future).
>>>>>>> That's true, but the example also demonstrates why it's kind of
>>>>>>> nonsensical. What do the 1, 2 represent for the purposes of
>>>>>>> __has_attribute?
>>>>>>
>>>>>> They represent themselves. Suppose we added support for a format
>>>>>> attribute
>>>>>> with negative indices, or with three indices, or something -- this
>>>>>> syntax
>>>>>> would allow the user to determine if that syntax is available.
>>>>>>
>>>>>>> Can they be elided? If so, can we come up with
>>>>>>> declarative rules as to when they can be elided?
>>>>>>
>>>>>> If you could omit them, how would you tell whether an attribute could be
>>>>>> used without arguments?
>>>>>>
>>>>>> Again, I'm not saying we should go in this direction, but I don't think
>>>>>> we
>>>>>> should dismiss it without consideration -- we probably don't want to
>>>>>> find we
>>>>>> need a third form of __has_attribute later =)
>>>>> That's one of the reasons Alp's suggestion for forwards compatibility
>>>>> is so nice -- if implemented properly, we could add parameter support
>>>>> at a later date (presuming we stick with the attribute syntax style
>>>>> approach).
>>>>>
>>>>> I'd like to avoid attempting to preprocess parameters for this patch,
>>>>> but had intended to leave the door open for the future. So it wasn't
>>>>> entirely without consideration. ;-)
>>>>
>>>> =) OK then!

-- 
http://www.nuanti.com
the browser experts