<div dir="ltr">On Fri, Aug 30, 2013 at 5:34 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Aug 30, 2013 at 8:06 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> Can you confirm my understanding:<br>
>  * for attributes that don't have an 'identifier, expression...' form, we<br>
> use getArgAsExpr and get an assert if the argument is not an expression.<br>
<br>
</div>Correct -- getArgAsExpr uses PorinterUnion::get internally, which asserts.<br>
<div class="im"><br>
>  * for attributes that might have an 'identifier, expression...' form, the<br>
> parser will sometimes give us an 'expression...' form instead, so those<br>
> cases always check isArgExpr.<br>
<br>
</div>Yes, I believe I've hit all of the places that need an isArgFoo check.<br>
 I added some test cases where I felt there wasn't good enough<br>
coverage, too.<br>
<div class="im"><br>
> (Given that, it seems surprising to me that alias, weakref, and the<br>
> 'handleAttrWithMessage' need the isArgExpr check. But it looks like that's<br>
> because we have some confusion over whether a StringArgument represents a<br>
> string literal or an identifier, and I don't want this patch blocked on<br>
> sorting that out!)<br>
<br>
</div>the isArgExpr check is for the case where the user does put in an<br>
identifier and we want to diagnose it.  Eg)<br>
<br>
__attribute__((alias(bad_thing)))<br>
<br>
vs<br>
<br>
__attribute__((alias("bad_thing")))</blockquote><div><br></div><div>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.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> You have "else if" after an "if" that returns in a few places in<br>
> SemaDeclAttr. We prefer to drop the 'else' in this case. Otherwise, LGTM.<br>
<br>
</div>Okay, I can fix those up.  Assuming those are corrected, are you fine<br>
with me committing?</blockquote><div><br></div><div>Yes. [For future reference, by "Otherwise, LGTM." I mean "Feel free to commit after making this minor tweak".]</div></div></div></div>