[cfe-dev] [PATCH] New syntax and functionality for __has_attribute
Alp Toker
alp at nuanti.com
Mon Feb 24 15:46:42 PST 2014
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
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
> <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> 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
> <aaron at aaronballman.com <mailto:aaron at aaronballman.com>>
> 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
> <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>>
> wrote:
>
> On Mon, Jan 13, 2014 at 5:44 PM, Aaron Ballman
> <aaron at aaronballman.com
> <mailto:aaron at aaronballman.com>>
> wrote:
>
> On Mon, Jan 13, 2014 at 8:39 PM, Richard Smith
> <richard at metafoo.co.uk
> <mailto:richard at metafoo.co.uk>>
> wrote:
>
> On Mon, Jan 13, 2014 at 5:31 PM, Aaron
> Ballman <aaron at aaronballman.com
> <mailto:aaron at aaronballman.com>>
> wrote:
>
> On Mon, Jan 13, 2014 at 8:20 PM,
> Richard Smith <richard at metafoo.co.uk
> <mailto:richard at metafoo.co.uk>>
> wrote:
>
> On Sun, Jan 12, 2014 at 4:49 PM,
> Aaron Ballman
> <aaron at aaronballman.com
> <mailto:aaron at aaronballman.com>>
> wrote:
>
> On Sun, Jan 12, 2014 at 7:38
> PM, Sean Silva
> <silvas at purdue.edu
> <mailto:silvas at purdue.edu>>
> wrote:
>
>
>
> On Sun, Jan 12, 2014 at
> 6:53 PM, Aaron Ballman
> <aaron at aaronballman.com
> <mailto:aaron at aaronballman.com>>
> 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
More information about the cfe-commits
mailing list