This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC PATCH] avoid applying attributes to explicit specializations (PR 83871)
On 02/06/2018 11:01 PM, Martin Sebor wrote:
On 02/05/2018 02:52 PM, Jason Merrill wrote:
On 02/04/2018 07:07 PM, Martin Sebor wrote:
To resolve the underlying root cause of the P1 bug c++/83503
- bogus -Wattributes for const and pure on function template
specialization, that we discussed last week, I've taken a stab
at making the change to avoid applying primary template's
attributes to its explicit specializations. (The bug tracking
the underlying root cause is 83871 - wrong code for attribute
const and pure on distinct template specializations).
The attached patch is what I have so far. It's incomplete
and not all the tests pass for a couple of reasons:
1) it only handles function templates (not class templates),
and I have no tests for those yet,
Class templates may already work the way you expect; at least aligned
does, though that doesn't involve TYPE_ATTRIBUTES.
Hmm, it seems that we currently don't propagate unused even to implicit
instantiations, a bug in the other direction:
template <class T> struct [[gnu::unused]] A { };
int main()
{
A<int> a; // shouldn't warn
}
I opened bug 84221 to track it. It's a regression WRT 4.7.
For types, it's not completely clear to me what should be
expected for attribute deprecated. Not inheriting the
attribute means that users would be able to explicitly
specialize a deprecated primary template which is in most
cases contrary to the intent of the attribute.
On the other hand, inheriting it means that there would be
no good way to deprecate the primary without also deprecating
its explicit specializations (because declaring the explicit
specializations would trigger the warning). The use case
for this was mentioned by Richard in the core discussion
(deprecating the std::numeric_limits primary).
I can't think of any way to make it work. The only solution
that comes to mind is to use the name of the source file (or
header) in which the primary is defined and allow explicit
specializations to be defined in it while issuing the warning
for those defined in other files. But this definitely seems
like GCC 9 material.
Yes, we definitely don't need to mess with this now.
2) it isn't effective for the nonnull and returns_nonnull
attributes because they are treated as type attributes,
3) the change to deal with attributes on function arguments
may be unnecessary (I couldn't come up with any that would
be propagated from the primary -- are there some?).
Yes, I think this is unnecessary.
Okay, thanks for confirming that.
Before I proceed with it I first want to make sure that it should
be fixed for GCC 8,
Some of it, I think. Probably not the whole issue.
that duplicate_decls is the right place for these changes
I think so.
and that (2) should be fixed by treating those
and other such attributes by applying them to function decls.
This seems out of bounds for GCC 8. It would also mean that we couldn't
use such attributes on pointers to functions.
+ TREE_NOTHROW (newdecl) |= TREE_NOTHROW (olddecl);
TREE_NOTHROW is mostly a non-attribute property, so I'd leave it out of
this.
__attribute__ ((nothrow))? The patch includes a test case with
wrong-code due to inheriting the attribute. With exception
specifications having to match between the primary and its
specializations it's the only way to make them different.
I've left this unchanged but let me know if I'm missing
something.
Yeah, I think you're right. But I notice that the existing code (and
thus your patch) touches TREE_NOTHROW in two places, and the first one
seems wrong; we only want it inside the FUNCTION_DECL section.
+ DECL_IS_MALLOC (olddecl) = DECL_IS_MALLOC (newdecl);
If a template is declared to be malloc, IMO we should really warn if a
specialization is missing that attribute, it seems certain to be a
mistake.
I tend to agree that it's likely a mistake. Though warning
in the front-end will lead to false positives if the function
isn't malloc. Ideally, this would be detected in the middle-
end (where -Wsuggest-attribute=malloc is handled) but I suspect
it's too late for that. I've added a simple warning for it.
In general, I think we should (optionally) warn if a template has
attributes and a specialization doesn't, as a user might have been
relying on the current behavior.
I've added a new option, -Wmissing-attribute. In bug 81824
Joseph asked for such a warning for C (for function resolvers
and aliases) and so I'll use the same option for both (I expect
it's too late to handle 81824 in GCC 8 but I'll finish it in
GCC 9). Adding the warning required passing some additional
attributes around and so more churn.
Rather than pass them down into register_specialization and
duplicate_decls, check_explicit_specialization could compare the
attribute list to the attributes on the template itself.
+ if (!merge_attr)
+ {
+ /* Remove the two function attributes that are, in fact,
+ treated as (quasi) type attributes. */
+ tree attrs = TYPE_ATTRIBUTES (newtype);
+ tree newattrs = remove_attribute ("nonnull", attrs);
+ newattrs = remove_attribute ("returns_nonnull", attrs);
+ if (newattrs != attrs)
+ TYPE_ATTRIBUTES (newtype) = newattrs;
+ }
Instead of this, we should avoid calling merge_types and just use
TREE_TYPE (newdecl) for newtype.
Ah, great, thanks. That works and fixes the outstanding FAILs
in the tests.
/* Merge the data types specified in the two decls. */
- newtype = merge_types (TREE_TYPE (newdecl), TREE_TYPE (olddecl));
+ newtype = TREE_TYPE (newdecl);
I meant to avoid merging only when !merge_attr; we should still merge if
merge_attr is true.
Attached is an updated patch. It hasn't gone through full
testing yet but please let me know if you'd like me to make
some changes.
+ const char* const whitelist[] = {
+ "error", "noreturn", "warning"
+ };
Why whitelist noreturn? I would expect to want that to be consistent.
Jason