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

Thu Mar 27 11:51:32 PDT 2014

We've started discussing a cross-vendor __has_attribute mechanism in the
C++ features SG, so I'd like us to hold off on any action here until that
discussion reaches consensus.

On Thu, Mar 27, 2014 at 11:48 AM, Aaron Ballman <aaron at aaronballman.com>wrote:

> On Sun, Mar 9, 2014 at 8:46 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Sat, Mar 8, 2014 at 2:51 PM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Tue, Feb 25, 2014 at 10:33 AM, Alp Toker <alp at nuanti.com> wrote:
> >> >
> >> > On 25/02/2014 15:13, Alp Toker wrote:
> >> >
> >> >
> >> > On 25/02/2014 13:45, Aaron Ballman wrote:
> >> >
> >> > 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,
> >> >
> >> >
> >> > Well, my take on this is that it's an established API that wasn't
> >> > designed
> >> > to be extensible :-)
> >> >
> >> > 2) it's a less awkward spelling
> >> >
> >> >
> >> > Granted, but that needs to be weighed up against the cost to users of
> >> > breaking backward compatibility.
> >> >
> >> > for the functionality, and 3) It's one less API for users to have to
> >> > keep in their brain.
> >> >
> >> >
> >> > Not really, it's three things to learn (bold emphasis on the points
> that
> >> > need to be learnt below):
> >> >
> >> > #if defined(__has_feature) && __has_feature(__has_attribute_syntax) &&
> >> > __has_attribute([[deprecated]])
> >> >
> >> > Instead of one thing to learn:
> >> >
> >> > #if defined(__has_attribute_syntax) &&
> >> > __has_attribute_syntax([[deprecated]])
> >> >
> >> > I prefer the latter because it matches the prescribed usage for all
> the
> >> > other feature detection macros:
> >> >
> >> >
> >> > #ifndef __has_builtin         // Optional of course.
> >> >   #define __has_builtin(x) 0  // Compatibility with non-clang
> compilers.
> >> > #endif
> >> >
> >> >
> >> > #ifndef __has_feature         // Optional of course.
> >> >   #define __has_feature(x) 0  // Compatibility with non-clang
> compilers.
> >> > #endif
> >> >
> >> > #if defined(__has_include)
> >> > #if __has_include("myinclude.h")
> >> > # include "myinclude.h"
> >> > #endif
> >> > #endif
> >> >
> >> > etc.
> >> >
> >> > Is the old name really that precious that we'd be willing to make
> >> > __has_attribute() a special case that's inconsistent with the other
> >> > feature
> >> > macros?
> >> >
> >> > 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.
> >> >
> >> >
> >> > Changing the semantics means pain for our users until those compilers
> go
> >> > away, and compilers have a tendency to stick around :-/
> >> >
> >> > Also we've been trying to clear up __has_feature()  (see my proposal
> on
> >> > C++
> >> > type trait detection) so it grates that we'd be adding another
> >> > non-language-standard check while at the same time trying to deprecate
> >> > the
> >> > existing ones:
> >> >
> >> > "__has_feature evaluates to 1 if the feature is both supported by
> Clang
> >> > and
> >> > standardized in the current language standard or 0 if not. For
> backwards
> >> > compatibility reasons, __has_feature can also be used to test for
> >> > support
> >> > for non-standardized features, i.e. features not prefixed c_, cxx_
> >> > orobjc_."
> >> >
> >> >
> >> > And I forgot to mention, changing the semantics of __has_attribute()
> >> > then
> >> > recommending users guard it with __has_feature(__has_attribute_syntax)
> >> > makes
> >> > it difficult for other vendors to implement the proposed feature
> >> > independently without also supporting a clang-style __has_feature()
> >> > detection macro. This is secondary but still worth keeping in mind.
> >> >
> >> >
> >> >
> >> > I don't mean to be contrary but I did want to write out my
> justification
> >> > in
> >> > full here from a language design perspective. If you know all this and
> >> > want
> >> > to go ahead, it's your module :-)
> >> >
> >> >
> >> > It may be in practice that the points raised don't matter much and I'd
> >> > like
> >> > to avoid confirmation bias -- perhaps Richard can tie-break?
> >>
> >> Ping?
> >
> >
> > Sorry it's taken me a while to circle back to this.
> >
> > It seems like the biggest point of contention is how this new feature
> > interacts with old Clang installations (or other compilers) that lack the
> > feature, and the discoverability of the relevant __has_feature check.
> >
> > Right now we recommend something like:
> >
> > #ifndef __has_attribute
> > #define __has_attribute(x) 0
> > #endif
> >
> > That'll work fine for non-Clang compilers and for new Clang, but not for
> old
> > Clang. If we use __has_attribute as the name of the new thing, old
> versions
> > of Clang will say:
> >
> > error: builtin feature check macro requires a parenthesized identifier
> >
> > ... which is pretty clear, even though it doesn't point to the relevant
> > __has_feature check. If we use __has_attribute_syntax, old versions of
> Clang
> > will diagnose the attribute *within* the parentheses, rather than the
> use of
> > __has_attribute_syntax itself. So it seems that keeping the same name
> leads
> > us to recover somewhat better.
> >
> > So I'm weakly in favor of extending __has_attribute rather than
> introducing
> > a new name for this functionality -- especially if we can also add a
> > -Wdeprecated warning for the old form of __has_attribute, suggesting a
> > migration to the new form.
>
> I kind of dropped the ball a bit on following up here, sorry about
> that. But it turns out I need this functionality in the parser as
> well, which reminded me to come back to this.
>
> This patch has been rebased against trunk, but it differs in terms of
> layering than the previous version. The previous version had a
> HasAttribute static helper function in PPMacroExpansion.cpp. This
> version exposes HasAttribute from the basic layer, which is a more
> sensible of a place for it given that I need to use it in the parser
> (and it's not really preprocessor-specific functionality). Note that
> the preprocessor parsing functionality remains in the preprocessor --
> the only thing that moved was the code checking whether the attribute
> is actually supported.
>
> Given Richard's tie-break, I think this feature is ready to commit
> (barring feedback, of course).
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140327/fbd46866/attachment.html>