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

[Richard Smith]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140309/bee5eb9e/attachment.html>