[prev in list] [next in list] [prev in thread] [next in thread]
List: gcc-patches
Subject: [C++] Weffc++/Wnon-virtual-dtor confusion
From: Nathan Sidwell <nathan () acm ! org>
Date: 2014-03-31 13:03:58
Message-ID: 533967BE.1020301 () acm ! org
[Download RAW message or body]
This patch fixes some unexpected behaviour of the Weffc++ and Wnon-virtual-dtor
flags. The documentation for the latter says it's also enabled by Weffc++, but
that's untrue.
The current behaviour of Weffc++ is to warn about any non-virtual dtor in a
non-polymorphic base class (relying on the below described -Wnon-virtual-dtor
case to catch polymorphic classes). Since Edition 3 of Scott Meyer's book, the
rule has been that it only applies to polymorphic classes. I.e. a polymorphic
class should not have an accessible non-virtual destructor and all of its bases
should be likewise. (it's possible earlier editions stated this rule too, but
3rd ed was what I could find).
The current behaviour of Wnon-virtual-dtor is to complain about accessible
non-virtual dtors in polymorphic classes. This is what one expects.
One surprising outcome of the current implementation is that -Weffc++
-Wno-non-virtual-dtor still complains about the lack of virtual dtors in base
classes.
This patch:
1) Clarifies the documentation for -Wnon-virtual-dtor to be for polymorphic
classes and bases of such classes. (i.e. move the 3rd ed Weffc++ behaviour into
this flag).
2) As different editions of Scott's book renumber the rules, I removed the
numbering in Weffc++ and merged the two separate lists.
3) Made -Wnon-virtual-dtor get set from -Weffc++.
4) Moved the check for nonvirtual dtors in base classes to after we determine
the class is polymorphic. Warn for all bases, not just the non-polymorphic ones.
5) Made that check trigger on -Wnon-virtual-dtor, not Weffc++.
I removed the 3 scattered tests for non-virtual dtor checking and updated and
cloned the warn/Wnvdtor.C test to check for the interactions between the two flags.
built & tested on i686-pc-linux-gnu. I'll commit in a few days, unless there
are comments.
nathan
["all.diff" (text/plain)]
2014-03-31 Nathan Sidwell <nathan@codesourcery.com>
doc/
* invoke.texi (Wnon-virtual-dtor): Adjust documentation.
(Weffc++): Remove Scott's numbering, merge lists and reference
Wnon-virtual-dtor.
c-family/
* c.opt (Wnon-virtual-dtor): Auto set when Weffc++.
cp/
* class.c (accessible_nvdtor_p): New.
(check_bases): Don't check base destructor here ...
(check_bases_and_members): ... check them here. Trigger on
Wnon-virtual-dtor flag/
(finish_struct_1): Use accessible_nvdtor_p.
testsuite/
* g++.dg/warn/Wnvdtor.C: Add non-polymorphic case.
* g++.dg/warn/Wnvdtor-2.C: New.
* g++.dg/warn/Wnvdtor-3.C: New.
* g++.dg/warn/Wnvdtor-4.C: New.
* g++.dg/warn/Weff1.C: Delete.
* g++.old-deja/g++.benjamin/15309-1.C: Delete.
* g++.old-deja/g++.benjamin/15309-2.C: Delete.
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt (revision 208870)
+++ gcc/c-family/c.opt (working copy)
@@ -569,7 +569,7 @@ C++ ObjC++ Var(warn_nontemplate_friend)
Warn when non-templatized friend functions are declared within a template
Wnon-virtual-dtor
-C++ ObjC++ Var(warn_nonvdtor) Warning
+C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
Warn about non-virtual destructors
Wnonnull
Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c (revision 208870)
+++ gcc/cp/class.c (working copy)
@@ -149,6 +149,7 @@ static tree *build_base_field (record_la
static void build_base_fields (record_layout_info, splay_tree, tree *);
static void check_methods (tree);
static void remove_zero_width_bit_fields (tree);
+static bool accessible_nvdtor_p (tree);
static void check_bases (tree, int *, int *);
static void check_bases_and_members (tree);
static tree create_vtable_ptr (tree, tree *);
@@ -1476,6 +1477,33 @@ inherit_targ_abi_tags (tree t)
mark_type_abi_tags (t, false);
}
+/* Return true, iff class T has a non-virtual destructor that is
+ accessible from outside the class heirarchy (i.e. is public, or
+ there's a suitable friend. */
+
+static bool
+accessible_nvdtor_p (tree t)
+{
+ tree dtor = CLASSTYPE_DESTRUCTORS (t);
+
+ /* An implicitly declared destructor is always public. And,
+ if it were virtual, we would have created it by now. */
+ if (!dtor)
+ return true;
+
+ if (DECL_VINDEX (dtor))
+ return false; /* Virtual */
+
+ if (!TREE_PRIVATE (dtor) && !TREE_PROTECTED (dtor))
+ return true; /* Public */
+
+ if (CLASSTYPE_FRIEND_CLASSES (t)
+ || DECL_FRIENDLIST (TYPE_MAIN_DECL (t)))
+ return true; /* Has friends */
+
+ return false;
+}
+
/* Run through the base classes of T, updating CANT_HAVE_CONST_CTOR_P,
and NO_CONST_ASN_REF_P. Also set flag bits in T based on
properties of the bases. */
@@ -1512,13 +1540,6 @@ check_bases (tree t,
if (!CLASSTYPE_LITERAL_P (basetype))
CLASSTYPE_LITERAL_P (t) = false;
- /* Effective C++ rule 14. We only need to check TYPE_POLYMORPHIC_P
- here because the case of virtual functions but non-virtual
- dtor is handled in finish_struct_1. */
- if (!TYPE_POLYMORPHIC_P (basetype))
- warning (OPT_Weffc__,
- "base class %q#T has a non-virtual destructor", basetype);
-
/* If the base class doesn't have copy constructors or
assignment operators that take const references, then the
derived class cannot have such a member automatically
@@ -5547,6 +5568,27 @@ 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
+ 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;
+ unsigned i;
+
+ for (binfo = TYPE_BINFO (t), i = 0;
+ BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
+ {
+ tree basetype = TREE_TYPE (base_binfo);
+
+ if (accessible_nvdtor_p (basetype))
+ warning (OPT_Wnon_virtual_dtor,
+ "base class %q#T has accessible non-virtual destructor",
+ basetype);
+ }
+ }
+
/* If the class has no user-declared constructor, but does have
non-static const or reference data members that can never be
initialized, issue a warning. */
@@ -6597,25 +6639,11 @@ finish_struct_1 (tree t)
/* This warning does not make sense for Java classes, since they
cannot have destructors. */
- if (!TYPE_FOR_JAVA (t) && warn_nonvdtor && TYPE_POLYMORPHIC_P (t))
- {
- tree dtor;
-
- dtor = CLASSTYPE_DESTRUCTORS (t);
- if (/* An implicitly declared destructor is always public. And,
- if it were virtual, we would have created it by now. */
- !dtor
- || (!DECL_VINDEX (dtor)
- && (/* public non-virtual */
- (!TREE_PRIVATE (dtor) && !TREE_PROTECTED (dtor))
- || (/* non-public non-virtual with friends */
- (TREE_PRIVATE (dtor) || TREE_PROTECTED (dtor))
- && (CLASSTYPE_FRIEND_CLASSES (t)
- || DECL_FRIENDLIST (TYPE_MAIN_DECL (t)))))))
- warning (OPT_Wnon_virtual_dtor,
- "%q#T has virtual functions and accessible"
- " non-virtual destructor", t);
- }
+ if (!TYPE_FOR_JAVA (t) && warn_nonvdtor
+ && TYPE_POLYMORPHIC_P (t) && accessible_nvdtor_p (t))
+ warning (OPT_Wnon_virtual_dtor,
+ "%q#T has virtual functions and accessible"
+ " non-virtual destructor", t);
complete_vars (t);
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 208870)
+++ gcc/doc/invoke.texi (working copy)
@@ -2670,9 +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, 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 also enabled if @option{-Weffc++} is specified.
+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.
@item -Wreorder @r{(C++ and Objective-C++ only)}
@opindex Wreorder
@@ -2716,40 +2717,34 @@ The following @option{-W@dots{}} options
@opindex Weffc++
@opindex Wno-effc++
Warn about violations of the following style guidelines from Scott Meyers'
-@cite{Effective C++, Second Edition} book:
+@cite{Effective C++} series of books:
@itemize @bullet
@item
-Item 11: Define a copy constructor and an assignment operator for classes
+Define a copy constructor and an assignment operator for classes
with dynamically-allocated memory.
@item
-Item 12: Prefer initialization to assignment in constructors.
+Prefer initialization to assignment in constructors.
@item
-Item 14: Make destructors virtual in base classes.
+Have @code{operator=} return a reference to @code{*this}.
@item
-Item 15: Have @code{operator=} return a reference to @code{*this}.
+Don't try to return a reference when you must return an object.
@item
-Item 23: Don't try to return a reference when you must return an object.
-
-@end itemize
-
-Also warn about violations of the following style guidelines from
-Scott Meyers' @cite{More Effective C++} book:
-
-@itemize @bullet
-@item
-Item 6: Distinguish between prefix and postfix forms of increment and
+Distinguish between prefix and postfix forms of increment and
decrement operators.
@item
-Item 7: Never overload @code{&&}, @code{||}, or @code{,}.
+Never overload @code{&&}, @code{||}, or @code{,}.
@end itemize
+This option also enables @option{-Wnon-virtual-dtor}, which is also
+one of the effective C++ recommendations.
+
When selecting this option, be aware that the standard library
headers do not obey all of these guidelines; use @samp{grep -v}
to filter out those warnings.
Index: gcc/testsuite/g++.dg/warn/Weff1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Weff1.C (revision 208870)
+++ gcc/testsuite/g++.dg/warn/Weff1.C (working copy)
@@ -1,5 +0,0 @@
-// { dg-options "-Weffc++" }
-
-struct S {};
-/* Base classes should have virtual destructors. */
-struct T : public S {}; // { dg-warning "" }
Index: gcc/testsuite/g++.dg/warn/Wnvdtor-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wnvdtor-2.C (revision 208870)
+++ gcc/testsuite/g++.dg/warn/Wnvdtor-2.C (working copy)
@@ -6,18 +6,18 @@
// destructor, in which case it would be possible but unsafe to delete
// an instance of a derived class through a pointer to the base class.
-struct A // { dg-bogus "non-virtual destructor" }
+struct A
{
protected:
- ~A();
+ ~A(); // inaccessible - no warning
public:
virtual void f() = 0;
};
-struct B // { dg-bogus "non-virtual destructor" }
+struct B
{
private:
- ~B();
+ ~B(); // inaccessible - no warning
public:
virtual void f() = 0;
};
@@ -52,3 +52,6 @@ private:
public:
virtual void f() = 0;
};
+
+struct H {};
+struct I : H {};
Index: gcc/testsuite/g++.dg/warn/Wnvdtor-3.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wnvdtor-3.C (revision 0)
+++ gcc/testsuite/g++.dg/warn/Wnvdtor-3.C (working copy)
@@ -0,0 +1,56 @@
+// { dg-do compile }
+// { dg-options "-Weffc++" }
+
+// Warn when a class has virtual functions and accessible non-virtual
+// destructor, in which case it would be possible but unsafe to delete
+// an instance of a derived class through a pointer to the base class.
+
+struct A
+{
+protected:
+ ~A(); // inaccessible - no warning
+public:
+ virtual void f() = 0;
+};
+
+struct B
+{
+private:
+ ~B(); // inaccessible - no warning
+public:
+ virtual void f() = 0;
+};
+
+struct C // { dg-warning "non-virtual destructor" }
+{
+ virtual void f() = 0;
+};
+
+struct D // { dg-warning "non-virtual destructor" }
+{
+ ~D();
+ virtual void f() = 0;
+};
+
+struct E;
+
+struct F // { dg-warning "non-virtual destructor" }
+{
+protected:
+ friend class E;
+ ~F();
+public:
+ virtual void f() = 0;
+};
+
+struct G // { dg-warning "non-virtual destructor" }
+{
+private:
+ friend class E;
+ ~G();
+public:
+ virtual void f() = 0;
+};
+
+struct H {};
+struct I : H {};
Index: gcc/testsuite/g++.dg/warn/Wnvdtor-4.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wnvdtor-4.C (revision 0)
+++ gcc/testsuite/g++.dg/warn/Wnvdtor-4.C (working copy)
@@ -0,0 +1,56 @@
+// { dg-do compile }
+// { dg-options "-Weffc++ -Wno-non-virtual-dtor" }
+
+// Warn when a class has virtual functions and accessible non-virtual
+// destructor, in which case it would be possible but unsafe to delete
+// an instance of a derived class through a pointer to the base class.
+
+struct A
+{
+protected:
+ ~A();
+public:
+ virtual void f() = 0;
+};
+
+struct B
+{
+private:
+ ~B();
+public:
+ virtual void f() = 0;
+};
+
+struct C
+{
+ virtual void f() = 0;
+};
+
+struct D
+{
+ ~D();
+ virtual void f() = 0;
+};
+
+struct E;
+
+struct F
+{
+protected:
+ friend class E;
+ ~F();
+public:
+ virtual void f() = 0;
+};
+
+struct G
+{
+private:
+ friend class E;
+ ~G();
+public:
+ virtual void f() = 0;
+};
+
+struct H {};
+struct I : H {};
Index: gcc/testsuite/g++.dg/warn/Wnvdtor.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wnvdtor.C (revision 208870)
+++ gcc/testsuite/g++.dg/warn/Wnvdtor.C (working copy)
@@ -8,3 +8,4 @@ extern "Java"
virtual void bar( void);
};
}
+
Index: gcc/testsuite/g++.old-deja/g++.benjamin/15309-1.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.benjamin/15309-1.C (revision 208870)
+++ gcc/testsuite/g++.old-deja/g++.benjamin/15309-1.C (working copy)
@@ -1,21 +0,0 @@
-// { dg-do assemble }
-// { dg-options "-Wnon-virtual-dtor -Weffc++" }
-// 981203 bkoz
-// g++/15309
-
-class bahamian {
-public:
- bahamian ();
- ~bahamian ();
-};
-
-class miami : public bahamian // { dg-warning "" } // WARNING -
-{
-public:
- miami ();
- ~miami ();
-};
-
-
-
-
Index: gcc/testsuite/g++.old-deja/g++.benjamin/15309-2.C
===================================================================
--- gcc/testsuite/g++.old-deja/g++.benjamin/15309-2.C (revision 208870)
+++ gcc/testsuite/g++.old-deja/g++.benjamin/15309-2.C (working copy)
@@ -1,10 +0,0 @@
-// { dg-do assemble }
-// { dg-options "-Wnon-virtual-dtor -Weffc++" }
-// 981203 bkoz
-// g++/15309
-
-class bermuda { // { dg-warning "" } // WARNING -
-public:
- virtual int func1(int);
- ~bermuda();
-};
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic