On Sun, Feb 23, 2014 at 10:08 PM, Alp Toker <[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)? > > 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
