From patchwork Mon Mar 31 13:03:58 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 335306 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 284701400A2 for ; Tue, 1 Apr 2014 00:04:15 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:content-type; q=dns; s=default; b=v5J364LfavLiUUnA0fw3yMOg6LXEIPRybrpsukAAhHw OIrj5NZb1dxDitgfzLFhJi+DYEXWq3Z2vmZADAFoKXlygcojAfS0Mo+ttyN6yLqH IkqPivK4glErkSo9HOh8TCpR1HupU2ZkOSM9FKVIz5ZCodfs1Ht/f7lyFTYmJwz8 = DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:content-type; s=default; bh=RyrqrGQ+aQweWPEr0BdxPGOxv2A=; b=Rw/Nq5ETcWdisAe96 6wJAMwIt9QW1aFKgCX61JNYJZchn8bu7F54/Pn96/qFaEOJq1U98mdTnMGLyn/U3 XRt/En559WTb65B+2h0fDppZYsbkZmNvoY4SuRn8pOEq71J4zjDjxxzxXcU5FCAm lI1FOxlCRpIwwkWOmPdTViA6Kk= Received: (qmail 11642 invoked by alias); 31 Mar 2014 13:04:08 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 11618 invoked by uid 89); 31 Mar 2014 13:04:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_05, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f177.google.com Received: from mail-wi0-f177.google.com (HELO mail-wi0-f177.google.com) (209.85.212.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 31 Mar 2014 13:04:04 +0000 Received: by mail-wi0-f177.google.com with SMTP id cc10so3257075wib.4 for ; Mon, 31 Mar 2014 06:04:00 -0700 (PDT) X-Received: by 10.180.12.43 with SMTP id v11mr12122755wib.33.1396271040605; Mon, 31 Mar 2014 06:04:00 -0700 (PDT) Received: from [192.168.44.107] ([90.204.148.40]) by mx.google.com with ESMTPSA id f3sm26614798wiv.2.2014.03.31.06.03.58 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 31 Mar 2014 06:03:59 -0700 (PDT) Message-ID: <533967BE.1020301@acm.org> Date: Mon, 31 Mar 2014 14:03:58 +0100 From: Nathan Sidwell User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: GCC Patches CC: Jason Merrill Subject: [C++] Weffc++/Wnon-virtual-dtor confusion X-IsSubscribed: yes 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 2014-03-31 Nathan Sidwell 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(); -};