[PATCH] Removing the notion of an attribute parameter

[Aaron Ballman]

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

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