[PATCH] Attribute parameters and arguments

[Richard Smith]

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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130909/0730b954/attachment.html>