On Fri, Feb 9, 2018 at 6:57 PM, Martin Sebor <mse...@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
