<div dir="ltr">On Mon, Sep 9, 2013 at 4:06 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">Made the three changes you suggested and attached -- but as for<br>
splitting out the constructors; there is a usage of the new<br>
functionality.  handleWeakRefAttr creates a WeakRefAttr that has no<br>
StringRef parameter.  However, that appears to be because WeakRefAttr<br>
is an AliasAttr in disguise sometimes.  If the optional argument is<br>
present, it becomes an AliasAttr, otherwise it stays a WeakRefAttr.<br>
What's more, nothing makes use of the WeakRefAttr that does take a<br>
StringRef parameter.  So this could be a place we use<br>
HasCustomParsing, but then again, the optional parameter constructor<br>
behavior handles this already.<br>
<br>
So basically:<br>
<br>
1) I could set HasCustomParsing for WeakRef, put back the semantic<br>
checks I removed, and remove the new constructors for optional<br>
parameters.<br>
or<br>
2) I could leave the new constructors in place and go with the patch<br>
as-attached.<br>
<br>
I'm fine either way.</blockquote><div><br></div><div>Ugh, I see. It's a bit nasty that we don't preserve the WeakRefAttr as written. :-/ It'd be nice to clear that up, and always store a weak_ref attribute as a WeakRefAttr. Either option works for me, and the new constructors make sense in general; patch LGTM.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Sep 9, 2013 at 6:26 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> Thanks!<br>
><br>
> HandleCommonAttributeFeatures: lowercase first letter.<br>
> SimpleArgument::writeCtorDefaultInitializers: maybe always just use the "()"<br>
> initializer?<br>
> You have a C++11 '>>' closing two templates on ClangAttrEmitter.cpp:1475.<br>
><br>
> The adding of the 'optional' bit and synthesis of new constructors seems to<br>
> be largely separate from this change (other than that you use the new bit to<br>
> determine whether you should perform an argument count check). In<br>
> particular, the new constructors don't seem to be used anywhere. Can you<br>
> split off that part of the change?<br>
><br>
><br>
> On Mon, Sep 9, 2013 at 2:54 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> (Bringing this back on the lists; it seems to have fallen off at some<br>
>> point.)<br>
>><br>
>> On Mon, Sep 9, 2013 at 5:07 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: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<br>
>> >> > <<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<br>
>> >> >> <<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<br>
>> >> >> >> attribute<br>
>> >> >> >> arguments, this is the original patch (sans parameter<br>
>> >> >> >> functionality)<br>
>> >> >> >> rebased against ToT.  It automates the error checking for the<br>
>> >> >> >> 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<br>
>> >> > 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<br>
>> >> > 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<br>
>> > 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<br>
>> > really<br>
>> > worked in C). But these days, it's more honest to say that each<br>
>> > 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<br>
>> > grammar<br>
>> > from the Args list.<br>
>><br>
>> 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>
>><br>
>> ~Aaron<br>
><br>
><br>
</div></div></blockquote></div><br></div></div>