On 25/02/2014 13:45, Aaron Ballman wrote:
On Mon, Feb 24, 2014 at 6:46 PM, Alp Toker <[email protected]> wrote:
On 24/02/2014 16:19, David Blaikie wrote:
On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <[email protected]
<mailto:[email protected]>> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
