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

[Aaron Ballman]

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?

~Aaron