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.reich...@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.reich...@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.reich...@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.reich...@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" }
+}
===================================================================
