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

[Aaron Ballman]

On Mon, Jan 13, 2014 at 1:03 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>
>
> On Mon, Jan 13, 2014 at 9:09 AM, Alp Toker <alp at nuanti.com> wrote:
>>
>>
>> On 13/01/2014 13:23, Aaron Ballman wrote:
>>
>> On Sun, Jan 12, 2014 at 11:43 PM, Kal <b17c0de at gmail.com> wrote:
>>
>> Am 13.01.14 03:43, schrieb Aaron Ballman:
>>
>> New patch attached with some suggestions implemented. Changes include:
>>
>> 1) Updated the cmake file because the name of the tablegen file was
>> updated (this should have been in the initial patch)
>> 2) Lexing unknown syntax now attempts better recovery. This allows us
>> to be a bit more future-proof
>> 3) Unknown syntax is now handled as a warning instead of an error.
>> However, malformed __has_attribute is still treated as an error (for
>> instance, __has_attribute[haha] will be an error, but
>> __has_attribute([haha]) will be a warning). The warning may warrant
>> its own diagnostic group instead of piggy-backing on UnknownAttribute.
>> 4) C++11 language option checking was moved into tablegen instead of
>> the preprocessor
>> 5) Updated test cases for the new warning, future-proofing and the
>> movement of C++11 language option checking
>>
>> What this patch does not address is:
>>
>> * It's still using __has_attribute while we discuss an improved name
>> * The default __has_attribute(ident) functionality still checks for
>> all attribute names (changing this would be a separate patch,
>> regardless)
>>
>> One other thing to consider with regards to naming -- another approach
>> we could take would be to not encode the syntax directly in the
>> feature macro, but instead use separate macros (with its analogous
>> currently-proposed syntax in comments):
>>
>> __has_gnu_attribute(ident)  // __has_attribute(__attribute__((ident)))
>> __has_declspec_attribute(ident)  // __has_attribute(__declspec(ident))
>> __has_generalized_attribute(ident) // __has_attribute([[ident]])
>> __has_keyword_attribute(ident)  // __has_attribute(ident)
>>
>>
>> What about the following instead (could also swap the order of these
>> arguments):
>>
>> __has_attribute(gnu,         ident) //
>> __has_attribute(__attribute__((ident)))
>> __has_attribute(declspec,    ident) // __has_attribute(__declspec(ident))
>> __has_attribute(generalized, ident) // __has_attribute([[ident]])
>> __has_attribute(keyword,     ident) // __has_attribute(ident)
>>
>> Thank you for the input!
>>
>> Unfortunately, it would cause backwards compatibility problems as the
>> current implementation has no fallback mode for syntaxes it doesn't
>> understand (so this would break existing code, and have no migration
>> path forward). At the very least, the name would have to change.
>> Personally, if we are using a single identifier for the feature test
>> macro, I would prefer to key off the distinct attribute syntax instead
>> of another contextualized identifier. Users already know how to write
>> the attribute, so the syntax is something already known. A new
>> contextual identifier is one more thing to learn and remember.
>>
>>
>> Yes, for the alternative proposal (3) it only makes sense to name the
>> macros individually for each attribute type. There's no value in expecting a
>> name in the first macro argument and the comma form would be incompatible.
>>
>> As a tweak to this latest alternative proposal, there's also no longer a
>> need for the spelling to be written because each macro can test for the
>> attribute name precisely.
>>
>> Also instead of 'ident', it's strictly speaking a 'name' in the case of
>> C++11 generalized attributes given that they can be namespaced (gnu, omp?).
>> These are the forms and names I'd suggest:
>>
>> __has_attribute(deprecated) // Test for GNU attributes only, macro always
>> defined (minor tightening of semantics)
>> __has_declspec(deprecated) // Macro only defined in MSVC extension mode
>> __has_generalized_attribute(gnu::deprecated) // Macro only defined in
>> C++11 mode
>> __has_simple_attribute(alignas) // Macro always defined, but mostly useful
>> for C++11 keyword attributes like 'alignas'
>>
>>
>> Overall I think it'll be either this or Aaron's proposal to have a single
>> unified feature macro that uses the spellings.
>
>
> (Playing devil's advocate here to try to get things off the fence)
>
> Is it possible for there to be a situation where e.g.
>
> gcc 4.8 supports __attribute__((foo(bar)))
> gcc 4.9 supports __attribute__((foo(bar))) and __attribute__((foo(bar,3)))
>
> In that case, something like __has_gnu_attribute(foo) wouldn't tell the
> whole story, but "__has_attribute_syntax" would be able to detect the
> presence.

The syntax-based approach leaves the door open to testing parameter
forms, but I'm not certain we'd ever want to go down that path in
practice because so many attribute parameters require contextual
information, and it seems like it would open the door to all sorts of
impossible-to answer questions.

> Btw, one point of confusion for the name `__has_attribute_syntax` is that it
> could be interpreted as asking "do we support generalized vs. gnu vs.
> declspec syntax" (independently of the actual attribute itself), rather than
> "do we support this attribute when written with this syntax". I.e.
> `__has_attribute_syntax([[foo]])` could be misinterpreted as being true when
> we support spelling attributes with generalized syntax (the "foo" is then
> just a placeholder), and false otherwise (this would be a terrible way to
> actually implement exposing that check, but that's not necessarily obvious
> to users).

True, but the user would likely catch that mistake pretty quickly (I
would hope) and not make it again.

> `__has_attribute_spelling([[foo]])` is slightly better in this regard, but
> still could be misinterpreted in that way.
> `__has_attribute_spelled([[foo]])` or
> `__has_attribute_with_spelling([[foo]])` or

Spelling for attributes is a bit different than the syntax. For
instance, you can have the same attribute "spelled" different ways.
Eg) AlignedAttr is spelled aligned, align, alignas and _Alignas (with
some differing syntaxes mixed in for good measure). So I'd want to
avoid using "spelling" in the name as that could also cause confusion.

> `__has_attribute_written([[foo]])` appear to avoid that misinterpretation.

I'd be amenable to that, but still slightly prefer _syntax since that
form follows function a bit more closely.

~Aaron