[PATCH] Initial support for Attribute subjects
Aaron Ballman
aaron at aaronballman.com
Wed Jul 10 13:53:54 PDT 2013
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
More information about the cfe-commits
mailing list