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
