On 04/06/14 10:50, Markus Trippelsdorf wrote:
On 2014.04.06 at 09:13 +0100, Nathan Sidwell wrote:
On 04/04/14 18:33, Nathan Sidwell wrote:
I'm testing a patch that makes the test in the loop:
if (TREE_PUBLIC (base_binfo)
Hm, binfo's aren't noted that way, it's encoded in BINFO_ACCESS and
SET_BINFO_ACCESS in search.c. We'd need to move those back to cp.h or expose an
interface in search.c. This is looking like a rat hole ...
That was my rustiness, it is not as complicated.
Here is a patch to implement the behaviour we discussed:
*) only consider public bases
*) only consider polymorphic bases, unless Weffc++ is also specified
I have tested this in the usual manner, and the new testcases pass with the
patch and fail without it.
Markus, if you could give this a try and see whether it fixes the problem you
reported, that'd be great.
Jason, I shall leave it to your discretion as to whether we should continue with
this patch, or revert the original one (for 4.9).
nathan
2014-04-07 Nathan Sidwell <nat...@codesourcery.com>
* doc/invoke (Wnon-virtual-dtor): Update to match implementation.
(Weffc++): Likewise.
cp/
* class.c (check_bases_and_members): Warn about non-virtual dtors
in public bases only. Check warn_eccp before complaining about
non-polymorphic bases.
testsuite/
* g++.dg/warn/Wnvdtor-2.C: Add more cases.
* g++.dg/warn/Wnvdtor-3.C: Likewise.
* g++.dg/warn/Wnvdtor-4.C: Likewise.
Index: cp/class.c
===================================================================
--- cp/class.c (revision 209122)
+++ cp/class.c (working copy)
@@ -5570,21 +5570,24 @@ check_bases_and_members (tree t)
TYPE_HAS_COMPLEX_MOVE_ASSIGN (t) |= TYPE_CONTAINS_VPTR_P (t);
TYPE_HAS_COMPLEX_DFLT (t) |= TYPE_CONTAINS_VPTR_P (t);
- /* Warn if a base of a polymorphic type has an accessible
+ /* Warn if a public base of a polymorphic type has an accessible
non-virtual destructor. It is only now that we know the class is
polymorphic. Although a polymorphic base will have a already
been diagnosed during its definition, we warn on use too. */
if (TYPE_POLYMORPHIC_P (t) && warn_nonvdtor)
{
- tree binfo, base_binfo;
+ tree binfo = TYPE_BINFO (t);
+ vec<tree, va_gc> *accesses = BINFO_BASE_ACCESSES (binfo);
+ tree base_binfo;
unsigned i;
- for (binfo = TYPE_BINFO (t), i = 0;
- BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
+ for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
{
tree basetype = TREE_TYPE (base_binfo);
- if (accessible_nvdtor_p (basetype))
+ if ((*accesses)[i] == access_public_node
+ && (TYPE_POLYMORPHIC_P (basetype) || warn_ecpp)
+ && accessible_nvdtor_p (basetype))
warning (OPT_Wnon_virtual_dtor,
"base class %q#T has accessible non-virtual destructor",
basetype);
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 209122)
+++ doc/invoke.texi (working copy)
@@ -2670,10 +2670,10 @@ the compiler to never throw an exception
@opindex Wnon-virtual-dtor
@opindex Wno-non-virtual-dtor
Warn when a class has virtual functions and an accessible non-virtual
-destructor itself or in a base class, or has in which case it is
-possible but unsafe to delete an instance of a derived class through a
-pointer to the base class. This warning is automatically enabled if
-@option{-Weffc++} is specified.
+destructor itself or in an accessible polymorphic base class, in which
+case it is possible but unsafe to delete an instance of a derived
+class through a pointer to the class itself or base class. This
+warning is automatically enabled if @option{-Weffc++} is specified.
@item -Wreorder @r{(C++ and Objective-C++ only)}
@opindex Wreorder
@@ -2743,7 +2743,9 @@ Never overload @code{&&}, @code{||}, or
@end itemize
This option also enables @option{-Wnon-virtual-dtor}, which is also
-one of the effective C++ recommendations.
+one of the effective C++ recommendations. However, the check is
+extended to warn about the lack of virtual destructor in accessible
+non-polymorphic bases classes too.
When selecting this option, be aware that the standard library
headers do not obey all of these guidelines; use @samp{grep -v}
Index: testsuite/g++.dg/warn/Wnvdtor-2.C
===================================================================
--- testsuite/g++.dg/warn/Wnvdtor-2.C (revision 209122)
+++ testsuite/g++.dg/warn/Wnvdtor-2.C (working copy)
@@ -54,4 +54,23 @@ public:
};
struct H {};
-struct I : H {};
+
+struct I1 : H
+{};
+struct I2 : private H
+{};
+
+struct J1 : H
+{ virtual ~J1 ();};
+struct J2 : private H
+{ virtual ~J2 ();};
+
+struct K // { dg-warning "accessible non-virtual destructor" }
+{
+ virtual void k ();
+};
+
+struct L1 : K // { dg-warning "accessible non-virtual destructor" }
+{virtual ~L1 ();};
+struct L2 : private K
+{virtual ~L2 ();};
Index: testsuite/g++.dg/warn/Wnvdtor-3.C
===================================================================
--- testsuite/g++.dg/warn/Wnvdtor-3.C (revision 209122)
+++ testsuite/g++.dg/warn/Wnvdtor-3.C (working copy)
@@ -53,4 +53,23 @@ public:
};
struct H {};
-struct I : H {};
+
+struct I1 : H
+{};
+struct I2 : private H
+{};
+
+struct J1 : H // { dg-warning "accessible non-virtual destructor" }
+{ virtual ~J1 ();};
+struct J2 : private H
+{ virtual ~J2 ();};
+
+struct K // { dg-warning "accessible non-virtual destructor" }
+{
+ virtual void k ();
+};
+
+struct L1 : K // { dg-warning "accessible non-virtual destructor" }
+{virtual ~L1 ();};
+struct L2 : private K
+{virtual ~L2 ();};
Index: testsuite/g++.dg/warn/Wnvdtor-4.C
===================================================================
--- testsuite/g++.dg/warn/Wnvdtor-4.C (revision 209122)
+++ testsuite/g++.dg/warn/Wnvdtor-4.C (working copy)
@@ -53,4 +53,23 @@ public:
};
struct H {};
-struct I : H {};
+
+struct I1 : H
+{};
+struct I2 : private H
+{};
+
+struct J1 : H
+{ virtual ~J1 ();};
+struct J2 : private H
+{ virtual ~J2 ();};
+
+struct K
+{
+ virtual void k ();
+};
+
+struct L1 : K
+{virtual ~L1 ();};
+struct L2 : private K
+{virtual ~L2 ();};
