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()?
That way the widely-recognised feature detection fallback pattern will
just work and you can drop the __has_feature workaround entirely. Users
will be able to use the familiar:
#ifndef __has_attribute_syntax
#define __has_attribute_syntax(...)
#endif
Doing this also means we can remove the old, ambiguous identifier-only
form of the check from the new version of the check. With this change to
your proposed patch we can ensure safe and compatible usage which is
important with vendor extensions like this.
Alp.
On 23/02/2014 16:24, Aaron Ballman wrote:
Ping
~Aaron
On Wed, Feb 19, 2014 at 1:05 PM, Aaron Ballman <[email protected]> wrote:
This is the latest version of this patch -- with some additional
features as requested. Specifically, this is using the existing
__has_attribute syntax in an extended style, and implements a
feature-test macro (__has_feature(__has_attribute_syntax)) which
allows you to determine whether the extended syntax is supported or
not. It adds some additional documentation explaining this.
~Aaron
On Thu, Jan 23, 2014 at 10:23 AM, Aaron Ballman <[email protected]> wrote:
Before I start in on this again, I wanted to make sure that there was
a consensus that this functionality was desirable. I believe the
answer (based on some conversations in IRC and here on the lists) was
tentatively "yes."
As far as I can tell, the work left to be done on this is to add a
feature test for __has_attribute_syntax, write the documentation for
it and that's about it? The syntax-based form is the preferable
nomenclature because it leaves the door open for testing parameters at
some point in the future, and with the __has_attribute_syntax feature
test, it is both backwards and forwards compatible.
~Aaron
On Mon, Jan 13, 2014 at 8:49 PM, Richard Smith <[email protected]> wrote:
On Mon, Jan 13, 2014 at 5:44 PM, Aaron Ballman <[email protected]>
wrote:
On Mon, Jan 13, 2014 at 8:39 PM, Richard Smith <[email protected]>
wrote:
On Mon, Jan 13, 2014 at 5:31 PM, Aaron Ballman <[email protected]>
wrote:
On Mon, Jan 13, 2014 at 8:20 PM, Richard Smith <[email protected]>
wrote:
On Sun, Jan 12, 2014 at 4:49 PM, Aaron Ballman
<[email protected]>
wrote:
On Sun, Jan 12, 2014 at 7:38 PM, Sean Silva <[email protected]>
wrote:
On Sun, Jan 12, 2014 at 6:53 PM, Aaron Ballman
<[email protected]>
wrote:
That's good news -- thanks for confirming.
The feature detection macro itself will still need to have a
different
name
(or some other mechanism) so it can be used compatibly with
existing
clang
deployments, because _has_attribute() currently emits a parse
error
instead
of gracefully returning 0 when passed the new argument syntax:
tmp/attr2.cpp:1:5: error: builtin feature check macro requires
a
parenthesized identifier
#if __has_attribute(__attribute__((weakref)))
^
Good catch. Unfortunately, __has_attribute is really the best
identifier for the macro, so I am loathe to let it go.
Due to the current design of __has_attribute, we can't get away
with
"
magic" by expanding the non-function-like form into a value that
could
be tested. So we would really have to pick a new name if we are
worried about this.
Suggestions on the name are welcome.
Ok, I'll bite:
__has_attribute_written_as([[foo]])
__has_attribute_syntax([[foo]])
__has_attribute_spelling([[foo]])
I kind of like __has_attribute_syntax, truth be told.
Have we given up on using the name __has_attribute too soon? Users of
the
new syntax could write:
// Probably already in project's porting header
#ifndef __has_feature
#define __has_feature(x) 0
#endif
#if __has_feature(__has_attribute_syntax)
#define MY_HAS_ATTRIBUTE(...) __has_attribute(__VA_ARGS__)
#else
#define MY_HAS_ATTRIBUTE(...) 0
#endif
If it's given a different name, they instead would write something
like:
#ifdef __has_attribute_syntax
#define MY_HAS_ATTRIBUTE(...) __has_attribute_syntax(__VA_ARGS__)
#else
#define MY_HAS_ATTRIBUTE(...) 0
#endif
So I don't think the change-in-syntax argument holds water.
So are you proposing that we would have a different name for the
purposes of the __has_feature macro? Eg)
__has_feature(__has_attribute_syntax) is 1 for the proposed
functionality, and 0 otherwise?
It's a possibility. It could be that a new name is a better approach,
but
both directions seem to be feasible.
I'll ponder; I rather like keeping the existing name.
By the same argument, it's possible to add extra arguments to
__has_attribute, if we have a __has_feature check for the new syntax.
Also, supporting arguments in the attributes is useful in some cases
--
it's
not true that they don't make sense in a feature-checking facility.
For
instance:
__has_attribute( __attribute__((format)) )
... doesn't tell me whether __attribute__((format, gnu_scanf, 1, 2)
will
work (and I'd expect that the format attribute will gain additional
archetypes in future).
That's true, but the example also demonstrates why it's kind of
nonsensical. What do the 1, 2 represent for the purposes of
__has_attribute?
They represent themselves. Suppose we added support for a format
attribute
with negative indices, or with three indices, or something -- this
syntax
would allow the user to determine if that syntax is available.
Can they be elided? If so, can we come up with
declarative rules as to when they can be elided?
If you could omit them, how would you tell whether an attribute could be
used without arguments?
Again, I'm not saying we should go in this direction, but I don't think
we
should dismiss it without consideration -- we probably don't want to
find we
need a third form of __has_attribute later =)
That's one of the reasons Alp's suggestion for forwards compatibility
is so nice -- if implemented properly, we could add parameter support
at a later date (presuming we stick with the attribute syntax style
approach).
I'd like to avoid attempting to preprocess parameters for this patch,
but had intended to leave the door open for the future. So it wasn't
entirely without consideration. ;-)
=) OK then!
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
