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.

>
>> 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.

>
>> +          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.

>> +      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.

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.

Martin