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

Thu Mar 27 11:48:23 PDT 2014

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 --------------
A non-text attachment was scrubbed...
Name: HasAttribute v2.patch
Type: application/octet-stream
Size: 26305 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140327/9884b54e/attachment.obj>