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

[Sean Silva]

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.

-- Sean Silva


>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140113/eacc4355/attachment.html>