I noticed that we have a diagnostic "err_repeat_attribute", but
currently only vecreturn is making use of it.
This seems like it is something we could generalize across attributes
relatively easily, but there appears to be no documentation (even in
the C++ standard) as to whether duplicate attributes are acceptable or
not for any given attribute.
As best I can tell, most attributes should diagnose a duplicate on the
same subject. The exceptions, as best I can gather, are:
alloc_size
format_arg
nonnull
ownership_holds
ownership_returns
ownership_takes
argument_with_type_tag
pointer_with_type_tag
Basically, anything which allows you to specify an argument index as
an attribute parameter seems like it could have a duplicate attribute.
Would it make sense to add a generalized duplicate attribute test in
SemaDeclAttr that emits an error when a duplicate is found, except for
the above cases?
~Aaron
[...]
argument_with_type_tag
pointer_with_type_tag
Yes, there can be multiple such attributes on a function. But we
could reject the case where two attributes have the same argument
index:
void A_func(void *ptr, void *tag, void *tag2)
__attribute__(( pointer_with_type_tag(a,1,2) ))
__attribute__(( pointer_with_type_tag(a,1,3) )); // expected-error
Dmitri
This is a good idea, but very difficult to implement generically from
SemaDeclAttr.cpp; you are given a parsed attribute and a decl, but the
decl has a semantic attribute object and the two do not have a direct
mapping.
I am talking more about the case where the mere presence of the
attribute is what's being guarded against, since there is a one to one
mapping of parsed attributes to semantic attributes (in a general
sense).
~Aaron
I noticed that we have a diagnostic “err_repeat_attribute”, but
currently only vecreturn is making use of it.
This seems like it is something we could generalize across attributes
relatively easily, but there appears to be no documentation (even in
the C++ standard) as to whether duplicate attributes are acceptable or
not for any given attribute.
For a standard c++11 attribute, there is usually (always?) a restriction that the attribute cannot be repeated in the same attribute-specifier.
To resolve this in a more generic way, I am thinking of table
generating some code on the parsed attributes that allows us to ask
"given a list of Attr instances, are there any conflicts you can
diagnose with this parsed attribute?" Eg)
bool AttributeList::conflictsWith(Sema &S, attr_iterator I,
attr_iterator E) const;
In this way, when SemaDeclAttr.cpp does common checking for a decl (or
statement, type, etc), we can ask the parsed attribute to diagnose any
problems it can, generically.
There are two situations I am thinking of initially:
1) Duplicate attributes. This applies pretty generally, and when
opt-out is required, we can use table gen to specify the opt-out
2) Mutually exclusive attributes. This doesn't apply as generally,
but is something we already have hand-written support for (cold vs hot
attributes), and is missing elsewhere (calling conventions immediately
spring to mind).
Before I do more concrete work in this direction, do you see any
particular problems with this approach? Or is there a better way you
would prefer to see employed?
Thanks!
~Aaron
To resolve this in a more generic way, I am thinking of table
generating some code on the parsed attributes that allows us to ask
"given a list of Attr instances, are there any conflicts you can
diagnose with this parsed attribute?" Eg)
bool AttributeList::conflictsWith(Sema &S, attr_iterator I,
attr_iterator E) const;
In this way, when SemaDeclAttr.cpp does common checking for a decl (or
statement, type, etc), we can ask the parsed attribute to diagnose any
problems it can, generically.
There are two situations I am thinking of initially:
1) Duplicate attributes. This applies pretty generally, and when
opt-out is required, we can use table gen to specify the opt-out
2) Mutually exclusive attributes. This doesn't apply as generally,
but is something we already have hand-written support for (cold vs hot
attributes), and is missing elsewhere (calling conventions immediately
spring to mind).
Before I do more concrete work in this direction, do you see any
particular problems with this approach? Or is there a better way you
would prefer to see employed?
Two things:
1) What you're proposing sounds O(n^2). While that's unlikely to be a
problem in practice, it'd be nice to avoid.
2) The C++ restriction is only for attributes *in the same
attribute-specifier*. So:
[[noreturn, noreturn]] void f(); // ill-formed
[[noreturn]] [[noreturn]] void f(); // ok
IIRC, we don't preserve enough information into Sema to be able to diagnose
this there.
To resolve this in a more generic way, I am thinking of table
generating some code on the parsed attributes that allows us to ask
"given a list of Attr instances, are there any conflicts you can
diagnose with this parsed attribute?" Eg)
bool AttributeList::conflictsWith(Sema &S, attr_iterator I,
attr_iterator E) const;
In this way, when SemaDeclAttr.cpp does common checking for a decl (or
statement, type, etc), we can ask the parsed attribute to diagnose any
problems it can, generically.
There are two situations I am thinking of initially:
1) Duplicate attributes. This applies pretty generally, and when
opt-out is required, we can use table gen to specify the opt-out
2) Mutually exclusive attributes. This doesn't apply as generally,
but is something we already have hand-written support for (cold vs hot
attributes), and is missing elsewhere (calling conventions immediately
spring to mind).
Before I do more concrete work in this direction, do you see any
particular problems with this approach? Or is there a better way you
would prefer to see employed?
Two things:
1) What you're proposing sounds O(n^2). While that's unlikely to be a
problem in practice, it'd be nice to avoid.
It would be nice to avoid...
2) The C++ restriction is only for attributes *in the same
attribute-specifier*. So:
[[noreturn, noreturn]] void f(); // ill-formed
[[noreturn]] [[noreturn]] void f(); // ok
IIRC, we don't preserve enough information into Sema to be able to diagnose
this there.
We don't seem to preserve enough information, but at the same time,
this could still be a useful warning either way?
~Aaron