<div dir="ltr">Thanks!<div><br></div><div>HandleCommonAttributeFeatures: lowercase first letter.</div><div>SimpleArgument::writeCtorDefaultInitializers: maybe always just use the "()" initializer?</div><div>You have a C++11 '>>' closing two templates on ClangAttrEmitter.cpp:1475.<br>
</div><div><br></div><div>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?</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 9, 2013 at 2:54 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(Bringing this back on the lists; it seems to have fallen off at some point.)<br>
<div><div class="h5"><br>
On Mon, Sep 9, 2013 at 5:07 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Mon, Sep 9, 2013 at 11:50 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Sep 9, 2013 at 2:39 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> wrote:<br>
>> > On Mon, Sep 9, 2013 at 11:31 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Sep 9, 2013 at 2:03 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> wrote:<br>
>> >> > On Mon, Sep 9, 2013 at 10:14 AM, Aaron Ballman<br>
>> >> > <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> Now that the notion of a parameter has been removed from attribute<br>
>> >> >> arguments, this is the original patch (sans parameter functionality)<br>
>> >> >> rebased against ToT.  It automates the error checking for the number<br>
>> >> >> of attribute args expected vs given.<br>
>> >> ><br>
>> >> ><br>
>> >> > I'm not a fan of the name 'CommonErrOptOut'. I think what we're<br>
>> >> > expressing<br>
>> >> > here is that the attribute has custom parsing rules (that is, the<br>
>> >> > parsing<br>
>> >> > rule isn't just "parse the arguments as specified by Args"). Maybe<br>
>> >> > 'HasCustomParsing'?<br>
>> >><br>
>> >> Hmm, it's a bit different than that to me.  More like<br>
>> >> HasCustomSemaChecking, since that's what it really specifies -- that<br>
>> >> sema checking cannot be automated.<br>
>> ><br>
>> ><br>
>> > I think what this flag means is that the Args list reflects the syntax<br>
>> > of<br>
>> > the attribute, not just its semantic contents. (It's unfortunate that<br>
>> > we've<br>
>> > muddled these up so much in the .td file, but it is at least convenient,<br>
>> > since they usually match exactly.)<br>
>><br>
>> So you would like to see this flag tied tightly to Args?  I was<br>
>> thinking it might be a more general flag for opting out of automated<br>
>> semantics checks, with the eventual goal being that the flag goes away<br>
>> once everything can be table driven.<br>
><br>
><br>
> I think of the argument count (and type) checks as being parsing checks<br>
> rather than semantic checks. It used to be the case that the syntax of the<br>
> attributes was basically<br>
><br>
>   attribute-name ( expression-list[opt] )<br>
><br>
> (with a weird special case for the first argument, which only ever really<br>
> worked in C). But these days, it's more honest to say that each attribute<br>
> has its own syntax:<br>
><br>
>   'address_space' ( constant-expression )<br>
>   'alias' ( string-literal )<br>
>   'aligned' ( constant-expression )<br>
>   'availability' ( identifier availability-arg-seq )<br>
>   ...<br>
><br>
> and this flag essentially says whether we can deduce the attribute's grammar<br>
> from the Args list.<br>
<br>
</div></div>Okay, that makes sense to me.  I've changed the patch to use<br>
HasCustomParsing instead.<br>
<br>
Updated patch attached.<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div>