[PATCH] Initial support for Attribute subjects

[Aaron Ballman]

On Wed, Jul 10, 2013 at 4:49 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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!).

Okay, I will come up with a more comprehensive patch that aims to replace:

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

with something a bit cleaner using the subjects.  I think the initial
patch will only cover a small handful of the attributes (the ones with
existing subjects perhaps) instead of going for all attributes.  Once
I have that, I'll post back to the list.

Thanks again!

~Aaron