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

[Alp Toker]

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_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_."

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?

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/9bed558a/attachment.html>