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

[Jason Merrill <jason at redhat dot com>]

On Fri, Feb 9, 2018 at 6:57 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/09/2018 12:52 PM, Jason Merrill wrote:
>> On 02/08/2018 04:52 PM, Martin Sebor wrote:
>>>
>>> I took me a while to find DECL_TEMPLATE_RESULT.  Hopefully
>>> that's the right way to get the primary from a TEMPLATE_DECL.
>>
>> Yes.
>>
>>>>> 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.
>>>
>>> I expect noreturn to be used on a primary whose definition
>>> is provided but that's not meant to be used the way the API
>>> is otherwise expected to be.  As in:
>>>
>>>    template <class T>
>>>    T [[noreturn]] foo () { throw "not implemented"; }
>>>
>>>    template <> int foo<int>();   // implemented elsewhere
>>
>> Marking that template as noreturn seems pointless, and possibly harmful;
>> the deprecated, warning, or error attributes would be better for this
>> situation.
>
> I meant either:
>
>   template <class T>
>   T __attribute__ ((noreturn)) foo () { throw "not implemented"; }
>
>   template <> int foo<int>();   // implemented elsewhere
>
> or (sigh)
>
>   template <class T>
>   [[noreturn]] T foo () { throw "not implemented"; }
>
>   template <> int foo<int>();   // implemented elsewhere
>
> It lets code like this
>
>   int bar ()
>   {
>      return foo<char>();
>   }
>
> be diagnosed because it's likely a bug (as Clang does with
> -Wunreachable-code).  It doesn't stop code like the following
> from compiling (which is good) but it instead lets them throw
> at runtime which is what foo's author wants.
>
>   void bar ()
>   {
>      foo<char>();
>   }
>
> It's the same as having an "unimplemented" base virtual function
> throw an exception when it's called rather than making it pure
> and having calls to it abort.  Declaring the base virtual function
> noreturn is useful for the same reason (and also diagnosed by
> Clang).  I should remember to add the same warning in GCC 9.

Yes, I understood the patterns you had in mind, but I disagree with
them.  My point about harmful is that declaring a function noreturn
because it's unimplemented could be a problem for when the function is
later implemented, and callers were optimized inappropriately.  This
seems like a rather roundabout way to get a warning about calling an
unimplemented function, and not worth overriding the normal behavior.

> I actually had some misgivings about both warning and deprecated
> for the white-listing, but not for noreturn.  My (only mild)
> concern is that both warning and deprecated functions can and
> likely will in some cases still be called, and so using them to
> suppress the warning runs the risk that their calls might be
> wrong and no one will notice.  Warning cannot be suppressed
> so it seems unlikely to be ignored, but deprecated can be.
> So I wonder if the white-listing for deprecated should be
> conditional on -Wdeprecated being enabled.
>
>>> -      /* Merge the type qualifiers.  */
>>> -      if (TREE_READONLY (newdecl))
>>> -    TREE_READONLY (olddecl) = 1;
>>>        if (TREE_THIS_VOLATILE (newdecl))
>>>      TREE_THIS_VOLATILE (olddecl) = 1;
>>> -      if (TREE_NOTHROW (newdecl))
>>> -    TREE_NOTHROW (olddecl) = 1;
>>> +
>>> +      if (merge_attr)
>>> +    {
>>> +      /* Merge the type qualifiers.  */
>>> +      if (TREE_READONLY (newdecl))
>>> +        TREE_READONLY (olddecl) = 1;
>>> +    }
>>> +      else
>>> +    {
>>> +      /* Set the bits that correspond to the const function
>>> attributes.  */
>>> +      TREE_READONLY (olddecl) = TREE_READONLY (newdecl);
>>> +    }
>>
>>
>> Let's limit the const/volatile handling here to non-functions, and
>> handle the const/noreturn attributes for functions in the later hunk
>> along with nothrow/malloc/pure.
>
>
> I had to keep the TREE_HAS_VOLATILE handling as is since it
> applies to functions too (has side-effects).  Otherwise the
> attr-nothrow-2.C test fails.

When I mentioned "noreturn" above I was referring to
TREE_HAS_VOLATILE; sorry I wasn't clear.  For functions it should be
handled along with nothrow/readonly/malloc/pure.

Jason