Re: [RFC PATCH] avoid applying attributes to explicit specializations (PR 83871)

[Jason Merrill <jason at redhat dot com>]

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
}

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

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

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

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.

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

Jason