On Fri, Aug 30, 2013 at 8:55 PM, Richard Smith <[email protected]> wrote: > On Fri, Aug 30, 2013 at 5:34 PM, Aaron Ballman <[email protected]> > wrote: >> >> On Fri, Aug 30, 2013 at 8:06 PM, Richard Smith <[email protected]> >> wrote: >> > Can you confirm my understanding: >> > * for attributes that don't have an 'identifier, expression...' form, >> > we >> > use getArgAsExpr and get an assert if the argument is not an expression. >> >> Correct -- getArgAsExpr uses PorinterUnion::get internally, which asserts. >> >> > * for attributes that might have an 'identifier, expression...' form, >> > the >> > parser will sometimes give us an 'expression...' form instead, so those >> > cases always check isArgExpr. >> >> Yes, I believe I've hit all of the places that need an isArgFoo check. >> I added some test cases where I felt there wasn't good enough >> coverage, too. >> >> > (Given that, it seems surprising to me that alias, weakref, and the >> > 'handleAttrWithMessage' need the isArgExpr check. But it looks like >> > that's >> > because we have some confusion over whether a StringArgument represents >> > a >> > string literal or an identifier, and I don't want this patch blocked on >> > sorting that out!) >> >> the isArgExpr check is for the case where the user does put in an >> identifier and we want to diagnose it. Eg) >> >> __attribute__((alias(bad_thing))) >> >> vs >> >> __attribute__((alias("bad_thing"))) > > > We should catch this in the parser, as we do for the other attributes that > don't take an identifier as the first argument. It looks like > attributeHasExprArgs is returning the wrong value for 'alias', 'weakref', > 'deprecated', 'unavailable', and probably a few other attributes. (I think > we should flip that around, so that accepting an identifier is something > that attributes opt into, rather than being the default, and require the > relevant attributes to take an IdentifierArgument in cases where they don't > already do so.)
Agreed. >> >> > You have "else if" after an "if" that returns in a few places in >> > SemaDeclAttr. We prefer to drop the 'else' in this case. Otherwise, >> > LGTM. >> >> Okay, I can fix those up. Assuming those are corrected, are you fine >> with me committing? > > > Yes. [For future reference, by "Otherwise, LGTM." I mean "Feel free to > commit after making this minor tweak".] I figure better safe than face the ire of Chandler. ;-) Thanks for the review and clarification. Committed in r189711 ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
