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
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.
Alp.
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] <mailto:[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] <mailto:[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] <mailto:[email protected]>>
wrote:
On Mon, Jan 13, 2014 at 5:44 PM, Aaron Ballman
<[email protected]
<mailto:[email protected]>>
wrote:
On Mon, Jan 13, 2014 at 8:39 PM, Richard Smith
<[email protected]
<mailto:[email protected]>>
wrote:
On Mon, Jan 13, 2014 at 5:31 PM, Aaron
Ballman <[email protected]
<mailto:[email protected]>>
wrote:
On Mon, Jan 13, 2014 at 8:20 PM,
Richard Smith <[email protected]
<mailto:[email protected]>>
wrote:
On Sun, Jan 12, 2014 at 4:49 PM,
Aaron Ballman
<[email protected]
<mailto:[email protected]>>
wrote:
On Sun, Jan 12, 2014 at 7:38
PM, Sean Silva
<[email protected]
<mailto:[email protected]>>
wrote:
On Sun, Jan 12, 2014 at
6:53 PM, Aaron Ballman
<[email protected]
<mailto:[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
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
