[PATCH] Removing the notion of an attribute parameter

[Richard Smith]

On Fri, Aug 30, 2013 at 5:34 PM, Aaron Ballman <aaron at aaronballman.com>wrote:

> On Fri, Aug 30, 2013 at 8:06 PM, Richard Smith <richard at metafoo.co.uk>
> 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.)


> > 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".]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130830/a09ff7a8/attachment.html>