[PATCH] Attribute parameters and arguments

[Aaron Ballman]

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.

~Aaron

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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AttrParamArgs3.patch
Type: application/octet-stream
Size: 45933 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130909/7d60f3ff/attachment.obj>