[PATCH] Initial support for Attribute subjects

[Richard Smith]

On Wed, Jul 10, 2013 at 1:16 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Wed, Jul 10, 2013 at 3:53 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> +      else if ((*I)->isSubClassOf("SubsetSubject")) {
>> +        std::string FN, TN;
>> +        bool IsDecl = writeAppertainsToSubjectSubsetDefinition(OS, *I, FN, TN);
>> +        std::string C = FN + "(cast<" + TN + ">(P))";
>>
>> This cast<> doesn't look right: we can get here if the potential
>> subject is not a TN, can't we?
>
> Good catch!  I assume the preference is for dyn_cast instead of
> dyn_cast_or_null?  I will also update the appertainsTo*** functions
> check for null input in the case dyn_cast fails.
>
>> How are you intending that process of creating and adding attributes
>> will work once this lands? Right now, the usual flow of handleFooAttr
>> is:
>>
>> For each entity where we want to apply the attribute:
>>  * Check attribute appertains to entity
>>  * Perform other semantic checks on the attribute and on whether it
>> can be attached to the entity
>>  * Create FooAttr instance and add it
>>
>> With appertainsTo being a method on FooAttr, that won't work. Perhaps
>> we can flip this around:
>>
>>  * Perform semantic checks on the attribute by itself
>>  * Create FooAttr from the AttributeList
>>  * For each entity where we want to apply the attribute:
>>     * Check whether it appertains to the entity
>>     * Perform semantic checks on the (attribute, entity) pair
>>     * Add it
>>
>> There are a couple of cases where we need to modify the attribute once
>> we see what it's being applied to; those don't look too hard to
>> support but are something to keep in mind.
>
> You are right, this would be slightly awkward since it requires the
> attribute instance itself.  But I'm not certain the attribute instance
> is strictly required -- what if these were static functions instead?
> So you could do:
>
> if (FooAttr::appertainsTo(something)) {
>
> }
>
> This would allow you to do:
>
> * Check appertainsTo on a specific attribute
> * Perform other semantic checks
> * Create FooAttr instance and add it
>
> The downside to this would be that you would no longer be able to do
> someAttrInst->appertainsTo(something), but that could be something
> easily added as an instance method if we felt it was beneficial at
> some point.
>
> Thoughts (and thanks for the review!)?

Suppose you generate static methods. How would you rework the existing
handleFooAttr code to use them? I suppose you could replace the
existing

static void handleFooAttr(...) {
  if (!isa<Subject1>(D) && !isa<Subject2>(D)) {
    S.Diag(... warn_attribute_wrong_decl_type) << ...;
    return;
  }

with

static void handleFooAttr(...) {
  if (!checkAppertainsTo<FooAttr>(...))
    return;

but I hope we can do a lot better than that. (Ideally, for simple
attributes, we shouldn't need to write a handleFooAttr function at
all.) Also, how will you produce the right diagnostics? Some
attributes want to customize their "does not appertain to this"
diagnostics in various ways.

I appreciate you keeping the patch minimal, but I think you need to
take it just a little further so that we can see that this direction
works out nicely (and so that you have test coverage!).