[PATCH] Attribute parameters and arguments

[Aaron Ballman]

Thanks for the review -- I've committed with the constructors in
r190368.  I'll add a FIXME to WeakRefAttr as well.

~Aaron

On Mon, Sep 9, 2013 at 7:16 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Mon, Sep 9, 2013 at 4:06 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Made the three changes you suggested and attached -- but as for
>> splitting out the constructors; there is a usage of the new
>> functionality.  handleWeakRefAttr creates a WeakRefAttr that has no
>> StringRef parameter.  However, that appears to be because WeakRefAttr
>> is an AliasAttr in disguise sometimes.  If the optional argument is
>> present, it becomes an AliasAttr, otherwise it stays a WeakRefAttr.
>> What's more, nothing makes use of the WeakRefAttr that does take a
>> StringRef parameter.  So this could be a place we use
>> HasCustomParsing, but then again, the optional parameter constructor
>> behavior handles this already.
>>
>> So basically:
>>
>> 1) I could set HasCustomParsing for WeakRef, put back the semantic
>> checks I removed, and remove the new constructors for optional
>> parameters.
>> or
>> 2) I could leave the new constructors in place and go with the patch
>> as-attached.
>>
>> I'm fine either way.
>
>
> 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.
>
>>
>> On Mon, Sep 9, 2013 at 6:26 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > Thanks!
>> >
>> > HandleCommonAttributeFeatures: lowercase first letter.
>> > SimpleArgument::writeCtorDefaultInitializers: maybe always just use the
>> > "()"
>> > initializer?
>> > You have a C++11 '>>' closing two templates on
>> > ClangAttrEmitter.cpp:1475.
>> >
>> > 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?
>> >
>> >
>> > On Mon, Sep 9, 2013 at 2:54 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> (Bringing this back on the lists; it seems to have fallen off at some
>> >> point.)
>> >>
>> >> On Mon, Sep 9, 2013 at 5:07 PM, Richard Smith <richard at metafoo.co.uk>
>> >> wrote:
>> >> > On Mon, Sep 9, 2013 at 11:50 AM, Aaron Ballman
>> >> > <aaron at aaronballman.com>
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Sep 9, 2013 at 2:39 PM, Richard Smith
>> >> >> <richard at metafoo.co.uk>
>> >> >> wrote:
>> >> >> > On Mon, Sep 9, 2013 at 11:31 AM, Aaron Ballman
>> >> >> > <aaron at aaronballman.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Mon, Sep 9, 2013 at 2:03 PM, Richard Smith
>> >> >> >> <richard at metafoo.co.uk>
>> >> >> >> wrote:
>> >> >> >> > On Mon, Sep 9, 2013 at 10:14 AM, Aaron Ballman
>> >> >> >> > <aaron at aaronballman.com>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> Now that the notion of a parameter has been removed from
>> >> >> >> >> attribute
>> >> >> >> >> arguments, this is the original patch (sans parameter
>> >> >> >> >> functionality)
>> >> >> >> >> rebased against ToT.  It automates the error checking for the
>> >> >> >> >> number
>> >> >> >> >> of attribute args expected vs given.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > I'm not a fan of the name 'CommonErrOptOut'. I think what we're
>> >> >> >> > expressing
>> >> >> >> > here is that the attribute has custom parsing rules (that is,
>> >> >> >> > the
>> >> >> >> > parsing
>> >> >> >> > rule isn't just "parse the arguments as specified by Args").
>> >> >> >> > Maybe
>> >> >> >> > 'HasCustomParsing'?
>> >> >> >>
>> >> >> >> Hmm, it's a bit different than that to me.  More like
>> >> >> >> HasCustomSemaChecking, since that's what it really specifies --
>> >> >> >> that
>> >> >> >> sema checking cannot be automated.
>> >> >> >
>> >> >> >
>> >> >> > I think what this flag means is that the Args list reflects the
>> >> >> > syntax
>> >> >> > of
>> >> >> > the attribute, not just its semantic contents. (It's unfortunate
>> >> >> > that
>> >> >> > we've
>> >> >> > muddled these up so much in the .td file, but it is at least
>> >> >> > convenient,
>> >> >> > since they usually match exactly.)
>> >> >>
>> >> >> So you would like to see this flag tied tightly to Args?  I was
>> >> >> thinking it might be a more general flag for opting out of automated
>> >> >> semantics checks, with the eventual goal being that the flag goes
>> >> >> away
>> >> >> once everything can be table driven.
>> >> >
>> >> >
>> >> > I think of the argument count (and type) checks as being parsing
>> >> > checks
>> >> > rather than semantic checks. It used to be the case that the syntax
>> >> > of
>> >> > the
>> >> > attributes was basically
>> >> >
>> >> >   attribute-name ( expression-list[opt] )
>> >> >
>> >> > (with a weird special case for the first argument, which only ever
>> >> > really
>> >> > worked in C). But these days, it's more honest to say that each
>> >> > attribute
>> >> > has its own syntax:
>> >> >
>> >> >   'address_space' ( constant-expression )
>> >> >   'alias' ( string-literal )
>> >> >   'aligned' ( constant-expression )
>> >> >   'availability' ( identifier availability-arg-seq )
>> >> >   ...
>> >> >
>> >> > and this flag essentially says whether we can deduce the attribute's
>> >> > grammar
>> >> > from the Args list.
>> >>
>> >> Okay, that makes sense to me.  I've changed the patch to use
>> >> HasCustomParsing instead.
>> >>
>> >> Updated patch attached.
>> >>
>> >> Thanks!
>> >>
>> >> ~Aaron
>> >
>> >
>
>