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

[Alp Toker]

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_featureevaluates 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_featurecan also be used to 
test for support for non-standardized features, i.e. features not 
prefixedc_,cxx_orobjc_."

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 :-)

Alp.

-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140225/e2e99640/attachment.html>