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 Mon, Feb 12, 2018 at 11:59 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/12/2018 09:30 AM, Jason Merrill wrote:
>>
>> 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.
>
>
> Removing noreturn from the whitelist means having to prevent
> the attribute from causing conflicts with the attributes on
> the blacklist. E.g., in this:
>
> template <class T> [[malloc]] void* allocate (int);
>
> template <> [[noreturn]] void* allocate<void> (int);
>
> -Wmissing-attributes would warn for the missing malloc but
> -Wattributes will warn once malloc is added. Ditto for all
> other attributes noreturn is considered to conflict with such
> as alloc_size and warn_unused_result.
This example seems rather unlikely, and the solution is to remove
[[noreturn]]. I don't think this is worth worrying about for GCC 8.
> I anticipate the warning code to ultimately end up in
> the middle-end so it can handle Joseph's case as well, and
> so it can also be integrated with the attribute conflict
> machinery. It also needs to be in the middle-end to become
> usable by -Wsuggest-attribute. But I wasn't thinking of
> making any of these bigger changes until GCC 9.
> Do you want me to integrate it with the conflict stuff now?
No, leaving it for GCC 9 makes sense to me.
Jason
>>> 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
>>
>