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

[Aaron Ballman]

On Mon, Jan 13, 2014 at 1:15 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>
>
> On Mon, Jan 13, 2014 at 1:12 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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.
>
>
> Cool. Just wanted to bring up this possible misinterpretation. I have no
> opinion on the right direction.

Thank you for bringing it up, it's a good point to raise.

~Aaron