On Mon, Feb 12, 2018 at 11:59 AM, Martin Sebor <mse...@gmail.com> wrote: > On 02/12/2018 09:30 AM, Jason Merrill wrote: >> >> 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. > > > 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 >> >