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