diff mbox

[C++] Weffc++/Wnon-virtual-dtor confusion

Message ID 53425620.30601@acm.org
State New
Headers show

Commit Message

Nathan Sidwell April 7, 2014, 7:39 a.m. UTC
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  <nathan@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.

Comments

Markus Trippelsdorf April 7, 2014, 8:48 a.m. UTC | #1
On 2014.04.07 at 08:39 +0100, Nathan Sidwell wrote:
> 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.

Well, all I've tested is if the warnings during an LLVM build vanish.
And this is indeed the case with your new patch, too.
Jason Merrill April 7, 2014, 1:22 p.m. UTC | #2
On 04/07/2014 03:39 AM, Nathan Sidwell wrote:
> 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).

This is OK.  Post 4.9 we might try removing the warn_ecpp check and see 
how that is received.

Jason
Nathan Sidwell April 8, 2014, 10:20 a.m. UTC | #3
On 04/07/14 14:22, Jason Merrill wrote:
> On 04/07/2014 03:39 AM, Nathan Sidwell wrote:
>> 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).
>
> This is OK.  Post 4.9 we might try removing the warn_ecpp check and see how that
> is received.

Thanks all, committed.
diff mbox

Patch

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 ();};