From patchwork Mon Apr 7 07:39:12 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 337411 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 550991400C6 for ; Mon, 7 Apr 2014 17:39:26 +1000 (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:references :in-reply-to:content-type; q=dns; s=default; b=DJ7A6g2C1zvF5jMV0 hUM7LlOQDYP6SR6hdI1u5O1CoZYGESqm9Titkim8A/DOZPtkgpa23iCTW3NcyR5t oOuwzPS/qBtmc6DS9iphe7Nh7zurJI6EkE0vaUScmhjs4u2FXDa2PIcnFydt+m77 6bWVRw3UrKwzn7iOy8g6irpx0I= 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:references :in-reply-to:content-type; s=default; bh=Y3anYBJTet5P8ZvL0Hrmaf4 q/fU=; b=tstfSMcVYEvPczrW7NGewHWV+I2UW55aDIHTYVfRi4bRp8BJGahC5Qk l9036POVc6JLyyntwp5h1QERooAH7O/uCkmPwnr0Hb5YN7cnt9fdWaVUp/rzEi4z NLRL7v8dlY7NAmOulSYZ6KzJZMJhqXfBU0A3WO04r19n29/8fNdU= Received: (qmail 25320 invoked by alias); 7 Apr 2014 07:39:20 -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 25310 invoked by uid 89); 7 Apr 2014 07:39:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-we0-f175.google.com Received: from mail-we0-f175.google.com (HELO mail-we0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 07 Apr 2014 07:39:17 +0000 Received: by mail-we0-f175.google.com with SMTP id q58so6256310wes.6 for ; Mon, 07 Apr 2014 00:39:14 -0700 (PDT) X-Received: by 10.180.89.102 with SMTP id bn6mr23913305wib.28.1396856354683; Mon, 07 Apr 2014 00:39:14 -0700 (PDT) Received: from [192.168.51.213] ([213.160.116.66]) by mx.google.com with ESMTPSA id fo6sm22111623wib.7.2014.04.07.00.39.13 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 07 Apr 2014 00:39:13 -0700 (PDT) Message-ID: <53425620.30601@acm.org> Date: Mon, 07 Apr 2014 08:39:12 +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: Markus Trippelsdorf CC: Jason Merrill , GCC Patches Subject: Re: [C++] Weffc++/Wnon-virtual-dtor confusion References: <533967BE.1020301@acm.org> <20140404163809.GA285@x4> <533EE25D.9090406@acm.org> <20140404165430.GB285@x4> <533EE635.4090106@acm.org> <533EE9BB.2030202@redhat.com> <533EECEA.6040805@acm.org> <53410CB9.6000704@acm.org> <20140406095033.GA283@x4> In-Reply-To: <20140406095033.GA283@x4> X-IsSubscribed: yes 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 * 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 *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 ();};