Thanks for the review -- I've committed with the constructors in r190368. I'll add a FIXME to WeakRefAttr as well.
~Aaron On Mon, Sep 9, 2013 at 7:16 PM, Richard Smith <[email protected]> wrote: > On Mon, Sep 9, 2013 at 4:06 PM, Aaron Ballman <[email protected]> > wrote: >> >> Made the three changes you suggested and attached -- but as for >> splitting out the constructors; there is a usage of the new >> functionality. handleWeakRefAttr creates a WeakRefAttr that has no >> StringRef parameter. However, that appears to be because WeakRefAttr >> is an AliasAttr in disguise sometimes. If the optional argument is >> present, it becomes an AliasAttr, otherwise it stays a WeakRefAttr. >> What's more, nothing makes use of the WeakRefAttr that does take a >> StringRef parameter. So this could be a place we use >> HasCustomParsing, but then again, the optional parameter constructor >> behavior handles this already. >> >> So basically: >> >> 1) I could set HasCustomParsing for WeakRef, put back the semantic >> checks I removed, and remove the new constructors for optional >> parameters. >> or >> 2) I could leave the new constructors in place and go with the patch >> as-attached. >> >> I'm fine either way. > > > Ugh, I see. It's a bit nasty that we don't preserve the WeakRefAttr as > written. :-/ It'd be nice to clear that up, and always store a weak_ref > attribute as a WeakRefAttr. Either option works for me, and the new > constructors make sense in general; patch LGTM. > >> >> On Mon, Sep 9, 2013 at 6:26 PM, Richard Smith <[email protected]> >> wrote: >> > Thanks! >> > >> > HandleCommonAttributeFeatures: lowercase first letter. >> > SimpleArgument::writeCtorDefaultInitializers: maybe always just use the >> > "()" >> > initializer? >> > You have a C++11 '>>' closing two templates on >> > ClangAttrEmitter.cpp:1475. >> > >> > The adding of the 'optional' bit and synthesis of new constructors seems >> > to >> > be largely separate from this change (other than that you use the new >> > bit to >> > determine whether you should perform an argument count check). In >> > particular, the new constructors don't seem to be used anywhere. Can you >> > split off that part of the change? >> > >> > >> > On Mon, Sep 9, 2013 at 2:54 PM, Aaron Ballman <[email protected]> >> > wrote: >> >> >> >> (Bringing this back on the lists; it seems to have fallen off at some >> >> point.) >> >> >> >> On Mon, Sep 9, 2013 at 5:07 PM, Richard Smith <[email protected]> >> >> wrote: >> >> > On Mon, Sep 9, 2013 at 11:50 AM, Aaron Ballman >> >> > <[email protected]> >> >> > wrote: >> >> >> >> >> >> On Mon, Sep 9, 2013 at 2:39 PM, Richard Smith >> >> >> <[email protected]> >> >> >> wrote: >> >> >> > On Mon, Sep 9, 2013 at 11:31 AM, Aaron Ballman >> >> >> > <[email protected]> >> >> >> > wrote: >> >> >> >> >> >> >> >> On Mon, Sep 9, 2013 at 2:03 PM, Richard Smith >> >> >> >> <[email protected]> >> >> >> >> wrote: >> >> >> >> > On Mon, Sep 9, 2013 at 10:14 AM, Aaron Ballman >> >> >> >> > <[email protected]> >> >> >> >> > wrote: >> >> >> >> >> >> >> >> >> >> Now that the notion of a parameter has been removed from >> >> >> >> >> attribute >> >> >> >> >> arguments, this is the original patch (sans parameter >> >> >> >> >> functionality) >> >> >> >> >> rebased against ToT. It automates the error checking for the >> >> >> >> >> number >> >> >> >> >> of attribute args expected vs given. >> >> >> >> > >> >> >> >> > >> >> >> >> > I'm not a fan of the name 'CommonErrOptOut'. I think what we're >> >> >> >> > expressing >> >> >> >> > here is that the attribute has custom parsing rules (that is, >> >> >> >> > the >> >> >> >> > parsing >> >> >> >> > rule isn't just "parse the arguments as specified by Args"). >> >> >> >> > Maybe >> >> >> >> > 'HasCustomParsing'? >> >> >> >> >> >> >> >> Hmm, it's a bit different than that to me. More like >> >> >> >> HasCustomSemaChecking, since that's what it really specifies -- >> >> >> >> that >> >> >> >> sema checking cannot be automated. >> >> >> > >> >> >> > >> >> >> > I think what this flag means is that the Args list reflects the >> >> >> > syntax >> >> >> > of >> >> >> > the attribute, not just its semantic contents. (It's unfortunate >> >> >> > that >> >> >> > we've >> >> >> > muddled these up so much in the .td file, but it is at least >> >> >> > convenient, >> >> >> > since they usually match exactly.) >> >> >> >> >> >> So you would like to see this flag tied tightly to Args? I was >> >> >> thinking it might be a more general flag for opting out of automated >> >> >> semantics checks, with the eventual goal being that the flag goes >> >> >> away >> >> >> once everything can be table driven. >> >> > >> >> > >> >> > I think of the argument count (and type) checks as being parsing >> >> > checks >> >> > rather than semantic checks. It used to be the case that the syntax >> >> > of >> >> > the >> >> > attributes was basically >> >> > >> >> > attribute-name ( expression-list[opt] ) >> >> > >> >> > (with a weird special case for the first argument, which only ever >> >> > really >> >> > worked in C). But these days, it's more honest to say that each >> >> > attribute >> >> > has its own syntax: >> >> > >> >> > 'address_space' ( constant-expression ) >> >> > 'alias' ( string-literal ) >> >> > 'aligned' ( constant-expression ) >> >> > 'availability' ( identifier availability-arg-seq ) >> >> > ... >> >> > >> >> > and this flag essentially says whether we can deduce the attribute's >> >> > grammar >> >> > from the Args list. >> >> >> >> Okay, that makes sense to me. I've changed the patch to use >> >> HasCustomParsing instead. >> >> >> >> Updated patch attached. >> >> >> >> Thanks! >> >> >> >> ~Aaron >> > >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
