public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Martin Sebor <msebor@gmail.com>
To: Volker Reichelt <v.reichelt@netcologne.de>, gcc-patches@gcc.gnu.org
Subject: Re: {PATCH] New C++ warning -Wcatch-value
Date: Mon, 15 May 2017 20:03:00 -0000	[thread overview]
Message-ID: <a04c422d-4ae4-31e9-105e-872cf79e2fa3@gmail.com> (raw)
In-Reply-To: <tkrat.8c7b4260a533be2f@netcologne.de>

> So how about the following then? I stayed with the catch part and added
> a parameter to the warning to let the user decide on the warnings she/he
> wants to get: -Wcatch-value=n.
> -Wcatch-value=1 only warns for polymorphic classes that are caught by
> value (to avoid slicing), -Wcatch-value=2 warns for all classes that
> are caught by value (to avoid copies). And finally -Wcatch-value=3
> warns for everything not caught by reference to find typos (like pointer
> instead of reference) and bad coding practices.

It seems reasonable to me.  I'm not too fond of multi-level
warnings since few users take advantage of anything but the
default, but this case is simple and innocuous enough that
I don't think it can do harm.

>
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK for trunk?
>
> If so, would it make sense to add -Wcatch-value=1 to -Wextra or even -Wall?
> I would do this in a seperate patch, becuase I haven't checked what that
> would mean for the testsuite.

I can't think of a use case for polymorphic slicing that's not
harmful so unless there is a common one that escapes me, I'd say
-Wall.

What are your thoughts on enhancing the warning to also handle
the rethrow case?

Also, it seems that a similar warning would be useful even beyond
catch handlers, to help detect slicing when passing arguments to
functions by value.  Especially in code that mixes OOP with the
STL (or other template libraries).  Have you thought about tackling
that at some point as well?

Martin

>
> Regards,
> Volker
>
>
> 2017-05-13  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* doc/invoke.texi (-Wcatch-value=): Document new warning option.
>
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi	(revision 248004)
> +++ gcc/doc/invoke.texi	(working copy)
> @@ -265,7 +265,7 @@
>  -Wno-builtin-declaration-mismatch @gol
>  -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
>  -Wc++-compat  -Wc++11-compat  -Wc++14-compat  -Wcast-align  -Wcast-qual  @gol
> --Wchar-subscripts -Wchkp  -Wclobbered  -Wcomment  @gol
> +-Wchar-subscripts  -Wchkp  -Wcatch-value=@var{n}  -Wclobbered  -Wcomment  @gol
>  -Wconditionally-supported  @gol
>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
>  -Wdelete-incomplete @gol
> @@ -5832,6 +5832,14 @@
>  literals to @code{char *}.  This warning is enabled by default for C++
>  programs.
>
> +@item -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)}
> +@opindex Wcatch-value
> +Warn about catch handlers that do not catch via reference.
> +With @option{-Wcatch-value=1} warn about polymorphic class types that
> +are caught by value. With @option{-Wcatch-value=2} warn about all class
> +types that are caught by value. With @option{-Wcatch-value=3} warn about
> +all types that are not caught by reference.
> +
>  @item -Wclobbered
>  @opindex Wclobbered
>  @opindex Wno-clobbered
> ===================================================================
>
> 2017-05-13  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* c.opt (Wcatch-value=): New C++ warning flag.
>
> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt	(revision 248004)
> +++ gcc/c-family/c.opt	(working copy)
> @@ -388,6 +388,10 @@
>  C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
>  Warn about casts which discard qualifiers.
>
> +Wcatch-value=
> +C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger
> +Warn about catch handlers of non-reference type.
> +
>  Wchar-subscripts
>  C ObjC C++ ObjC++ Var(warn_char_subscripts) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
>  Warn about subscripts whose type is \"char\".
> ===================================================================
>
> 2017-05-13  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* semantics.c (finish_handler_parms): Warn about non-reference type
> 	catch handlers.
>
> Index: gcc/cp/semantics.c
> ===================================================================
> --- gcc/cp/semantics.c	(revision 248004)
> +++ gcc/cp/semantics.c	(working copy)
> @@ -1321,7 +1321,28 @@
>  	}
>      }
>    else
> -    type = expand_start_catch_block (decl);
> +    {
> +      type = expand_start_catch_block (decl);
> +      if (warn_catch_value
> +	  && type != NULL_TREE
> +	  && type != error_mark_node
> +	  && TREE_CODE (TREE_TYPE (decl)) != REFERENCE_TYPE)
> +	{
> +	  tree orig_type = TREE_TYPE (decl);
> +	  if (CLASS_TYPE_P (orig_type))
> +	    {
> +	      if (TYPE_POLYMORPHIC_P (orig_type))
> +		warning (OPT_Wcatch_value_,
> +			 "catching polymorphic type %q#T by value", orig_type);
> +	      else if (warn_catch_value > 1)
> +		warning (OPT_Wcatch_value_,
> +			 "catching type %q#T by value", orig_type);
> +	    }
> +	  else if (warn_catch_value > 2)
> +	    warning (OPT_Wcatch_value_,
> +		     "catching non-reference type %q#T", orig_type);
> +	}
> +    }
>    HANDLER_TYPE (handler) = type;
>  }
>
> ===================================================================
>
> 2017-05-13  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* g++.dg/warn/Wcatch-value-1.C: New test.
> 	* g++.dg/warn/Wcatch-value-2.C: New test.
> 	* g++.dg/warn/Wcatch-value-3.C: New test.
>
> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-13
> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-13
> @@ -0,0 +1,64 @@
> +// { dg-options "-Wcatch-value=1" }
> +
> +struct A { virtual ~A() {} };
> +struct B : A {};
> +struct C {};
> +struct D : C {};
> +
> +void foo()
> +{
> +  try {}
> +  catch (D)    {}
> +  catch (C)    {}
> +  catch (B)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A*)   {}
> +  catch (int)  {}
> +
> +  try {}
> +  catch (D&)   {}
> +  catch (C&)   {}
> +  catch (B&)   {}
> +  catch (A&)   {}
> +  catch (A*)   {}
> +  catch (int&) {}
> +}
> +
> +template<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching polymorphic type" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();
> +  foo1<A>();  // { dg-message "required" }
> +  foo1<B>();  // { dg-message "required" }
> +  foo1<C>();
> +  foo1<D>();
> +
> +  foo2<int>();
> +  foo2<A>();
> +  foo2<B>();
> +  foo2<C>();
> +  foo2<D>();
> +}
> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-2.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wcatch-value-2.C	2017-05-13
> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-2.C	2017-05-13
> @@ -0,0 +1,64 @@
> +// { dg-options "-Wcatch-value=2" }
> +
> +struct A { virtual ~A() {} };
> +struct B : A {};
> +struct C {};
> +struct D : C {};
> +
> +void foo()
> +{
> +  try {}
> +  catch (D)    {}  // { dg-warning "catching type" }
> +  catch (C)    {}  // { dg-warning "catching type" }
> +  catch (B)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A*)   {}
> +  catch (int)  {}
> +
> +  try {}
> +  catch (D&)   {}
> +  catch (C&)   {}
> +  catch (B&)   {}
> +  catch (A&)   {}
> +  catch (A*)   {}
> +  catch (int&) {}
> +}
> +
> +template<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();
> +  foo1<A>();  // { dg-message "required" }
> +  foo1<B>();  // { dg-message "required" }
> +  foo1<C>();  // { dg-message "required" }
> +  foo1<D>();  // { dg-message "required" }
> +
> +  foo2<int>();
> +  foo2<A>();
> +  foo2<B>();
> +  foo2<C>();
> +  foo2<D>();
> +}
> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-3.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wcatch-value-3.C	2017-05-13
> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-3.C	2017-05-13
> @@ -0,0 +1,64 @@
> +// { dg-options "-Wcatch-value=3" }
> +
> +struct A { virtual ~A() {} };
> +struct B : A {};
> +struct C {};
> +struct D : C {};
> +
> +void foo()
> +{
> +  try {}
> +  catch (D)    {}  // { dg-warning "catching type" }
> +  catch (C)    {}  // { dg-warning "catching type" }
> +  catch (B)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A)    {}  // { dg-warning "catching polymorphic type" }
> +  catch (A*)   {}  // { dg-warning "catching non-reference type" }
> +  catch (int)  {}  // { dg-warning "catching non-reference type" }
> +
> +  try {}
> +  catch (D&)   {}
> +  catch (C&)   {}
> +  catch (B&)   {}
> +  catch (A&)   {}
> +  catch (A*)   {}  // { dg-warning "catching non-reference type" }
> +  catch (int&) {}
> +}
> +
> +template<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}  // { dg-warning "catching non-reference type" }
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();  // { dg-message "required" }
> +  foo1<A>();    // { dg-message "required" }
> +  foo1<B>();    // { dg-message "required" }
> +  foo1<C>();    // { dg-message "required" }
> +  foo1<D>();    // { dg-message "required" }
> +
> +  foo2<int>();  // { dg-message "required" }
> +  foo2<A>();    // { dg-message "required" }
> +  foo2<B>();    // { dg-message "required" }
> +  foo2<C>();    // { dg-message "required" }
> +  foo2<D>();    // { dg-message "required" }
> +}
> ===================================================================
>

  reply	other threads:[~2017-05-15 19:58 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-01  8:38 Volker Reichelt
2017-05-03  3:34 ` Martin Sebor
2017-05-07 20:28   ` Volker Reichelt
2017-05-08  3:18     ` Martin Sebor
2017-05-08 13:14       ` Jason Merrill via gcc-patches
2017-05-14 15:30       ` Volker Reichelt
2017-05-15 20:03         ` Martin Sebor [this message]
2017-05-24 20:13           ` Jason Merrill
2017-05-30  6:22             ` Volker Reichelt
2017-05-31 20:55               ` Jason Merrill
2017-06-05 10:55           ` Volker Reichelt
2017-06-05 19:08             ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a04c422d-4ae4-31e9-105e-872cf79e2fa3@gmail.com \
    --to=msebor@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=v.reichelt@netcologne.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).