Hi Argyrios and Matthieu,<div><br></div><div>this warning found a few problems in chromium – thanks! However, it also finds a few false positives, so I don't think I can turn it on by default, which is a bummer.</div><div>
<br></div><div>All of the false positives are of this form:</div><div><br></div><div>class SomeInterface {</div><div> public:</div><div> virtual void interfaceMethod() {} // or = 0;</div><div> protected:</div><div> ~SomeInterface() {}</div>
<div>}</div><div><br></div><div>class WorkerClass : public SomeInterface {</div><div> public:</div><div> // many non-virtual functions, but also:</div><div> virtual void interfaceMethod() override { /* do actual work */ }</div>
<div>};</div><div><br></div><div>void f() {</div><div> scoped_ptr<WorkerClass> c(new WorkerClass); // simplified example</div><div>}</div><div><br></div><div>This is a somewhat standard pattern (see e.g. <a href="http://www.gotw.ca/publications/mill18.htm">http://www.gotw.ca/publications/mill18.htm</a>, "Virtual Question #2").</div>
<div><br></div><div>Do you have any good suggestions how to deal with this case?<br><br></div><div>Thanks,</div><div>Nico<br><br><div class="gmail_quote">On Tue, May 24, 2011 at 12:53 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com">akyrtzi@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Author: akirtzidis<br>
Date: Tue May 24 14:53:26 2011<br>
New Revision: 131989<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=131989&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=131989&view=rev</a><br>
Log:<br>
Add new warning that warns when invoking 'delete' on a polymorphic, non-final, class without a virtual destructor.<br>
<br>
Patch by Matthieu Monrocq!<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
cfe/trunk/test/SemaCXX/destructor.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=131989&r1=131988&r2=131989&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=131989&r1=131988&r2=131989&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue May 24 14:53:26 2011<br>
@@ -35,6 +35,8 @@<br>
def : DiagGroup<"declaration-after-statement">;<br>
def GNUDesignator : DiagGroup<"gnu-designator">;<br>
<br>
+def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor">;<br>
+<br>
def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;<br>
def DeprecatedWritableStr : DiagGroup<"deprecated-writable-strings">;<br>
def Deprecated : DiagGroup<"deprecated", [ DeprecatedDeclarations] >,<br>
@@ -235,6 +237,7 @@<br>
def Most : DiagGroup<"most", [<br>
CharSubscript,<br>
Comment,<br>
+ DeleteNonVirtualDtor,<br>
Format,<br>
Implicit,<br>
MismatchedTags,<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=131989&r1=131988&r2=131989&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=131989&r1=131988&r2=131989&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue May 24 14:53:26 2011<br>
@@ -3069,6 +3069,9 @@<br>
def warn_non_virtual_dtor : Warning<<br>
"%0 has virtual functions but non-virtual destructor">,<br>
InGroup<NonVirtualDtor>, DefaultIgnore;<br>
+def warn_delete_non_virtual_dtor : Warning<<br>
+ "delete called on %0 that has virtual functions but non-virtual destructor">,<br>
+ InGroup<DeleteNonVirtualDtor>, DefaultIgnore;<br>
def warn_overloaded_virtual : Warning<<br>
"%q0 hides overloaded virtual %select{function|functions}1">,<br>
InGroup<OverloadedVirtual>, DefaultIgnore;<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=131989&r1=131988&r2=131989&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=131989&r1=131988&r2=131989&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Tue May 24 14:53:26 2011<br>
@@ -1800,6 +1800,20 @@<br>
const_cast<CXXDestructorDecl*>(Dtor));<br>
DiagnoseUseOfDecl(Dtor, StartLoc);<br>
}<br>
+<br>
+ // C++ [expr.delete]p3:<br>
+ // In the first alternative (delete object), if the static type of the<br>
+ // object to be deleted is different from its dynamic type, the static<br>
+ // type shall be a base class of the dynamic type of the object to be<br>
+ // deleted and the static type shall have a virtual destructor or the<br>
+ // behavior is undefined.<br>
+ //<br>
+ // Note: a final class cannot be derived from, no issue there<br>
+ if (!ArrayForm && RD->isPolymorphic() && !RD->hasAttr<FinalAttr>()) {<br>
+ CXXDestructorDecl *dtor = RD->getDestructor();<br>
+ if (!dtor || !dtor->isVirtual())<br>
+ Diag(StartLoc, diag::warn_delete_non_virtual_dtor) << PointeeElem;<br>
+ }<br>
}<br>
<br>
if (!OperatorDelete) {<br>
<br>
Modified: cfe/trunk/test/SemaCXX/destructor.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/destructor.cpp?rev=131989&r1=131988&r2=131989&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/destructor.cpp?rev=131989&r1=131988&r2=131989&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/destructor.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/destructor.cpp Tue May 24 14:53:26 2011<br>
@@ -1,4 +1,4 @@<br>
-// RUN: %clang_cc1 -fsyntax-only -Wnon-virtual-dtor -verify %s<br>
+// RUN: %clang_cc1 -std=c++0x -fsyntax-only -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -verify %s<br>
class A {<br>
public:<br>
~A();<br>
@@ -173,6 +173,179 @@<br>
TS2<int> foo; // expected-note {{instantiation}}<br>
}<br>
<br>
+namespace dnvd { // delete-non-virtual-dtor warning<br>
+struct NP {};<br>
+<br>
+struct B { // expected-warning {{has virtual functions but non-virtual destructor}}<br>
+ virtual void foo();<br>
+};<br>
+<br>
+struct D: B {}; // expected-warning {{has virtual functions but non-virtual destructor}}<br>
+<br>
+struct F final: B {}; // expected-warning {{has virtual functions but non-virtual destructor}}<br>
+<br>
+struct VB {<br>
+ virtual void foo();<br>
+ virtual ~VB();<br>
+};<br>
+<br>
+struct VD: VB {};<br>
+<br>
+struct VF final: VB {};<br>
+<br>
+template <typename T><br>
+class simple_ptr {<br>
+public:<br>
+ simple_ptr(T* t): _ptr(t) {}<br>
+ ~simple_ptr() { delete _ptr; } // \<br>
+ // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}} \<br>
+ // expected-warning {{delete called on 'dnvd::D' that has virtual functions but non-virtual destructor}}<br>
+ T& operator*() const { return *_ptr; }<br>
+private:<br>
+ T* _ptr;<br>
+};<br>
+<br>
+template <typename T><br>
+class simple_ptr2 {<br>
+public:<br>
+ simple_ptr2(T* t): _ptr(t) {}<br>
+ ~simple_ptr2() { delete _ptr; } // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}<br>
+ T& operator*() const { return *_ptr; }<br>
+private:<br>
+ T* _ptr;<br>
+};<br>
+<br>
+void use(B&);<br>
+void use(VB&);<br>
+<br>
+void nowarnstack() {<br>
+ B b; use(b);<br>
+ D d; use(d);<br>
+ F f; use(f);<br>
+ VB vb; use(vb);<br>
+ VD vd; use(vd);<br>
+ VF vf; use(vf);<br>
+}<br>
+<br>
+void nowarnnonpoly() {<br>
+ {<br>
+ NP* np = new NP();<br>
+ delete np;<br>
+ }<br>
+ {<br>
+ NP* np = new NP[4];<br>
+ delete[] np;<br>
+ }<br>
+}<br>
+<br>
+void nowarnarray() {<br>
+ {<br>
+ B* b = new B[4];<br>
+ delete[] b;<br>
+ }<br>
+ {<br>
+ D* d = new D[4];<br>
+ delete[] d;<br>
+ }<br>
+ {<br>
+ VB* vb = new VB[4];<br>
+ delete[] vb;<br>
+ }<br>
+ {<br>
+ VD* vd = new VD[4];<br>
+ delete[] vd;<br>
+ }<br>
+}<br>
+<br>
+template <typename T><br>
+void nowarntemplate() {<br>
+ {<br>
+ T* t = new T();<br>
+ delete t;<br>
+ }<br>
+ {<br>
+ T* t = new T[4];<br>
+ delete[] t;<br>
+ }<br>
+}<br>
+<br>
+void nowarn0() {<br>
+ {<br>
+ F* f = new F();<br>
+ delete f;<br>
+ }<br>
+ {<br>
+ VB* vb = new VB();<br>
+ delete vb;<br>
+ }<br>
+ {<br>
+ VB* vb = new VD();<br>
+ delete vb;<br>
+ }<br>
+ {<br>
+ VD* vd = new VD();<br>
+ delete vd;<br>
+ }<br>
+ {<br>
+ VF* vf = new VF();<br>
+ delete vf;<br>
+ }<br>
+}<br>
+<br>
+void warn0() {<br>
+ {<br>
+ B* b = new B();<br>
+ delete b; // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}<br>
+ }<br>
+ {<br>
+ B* b = new D();<br>
+ delete b; // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}<br>
+ }<br>
+ {<br>
+ D* d = new D();<br>
+ delete d; // expected-warning {{delete called on 'dnvd::D' that has virtual functions but non-virtual destructor}}<br>
+ }<br>
+}<br>
+<br>
+void nowarn1() {<br>
+ {<br>
+ simple_ptr<F> f(new F());<br>
+ use(*f);<br>
+ }<br>
+ {<br>
+ simple_ptr<VB> vb(new VB());<br>
+ use(*vb);<br>
+ }<br>
+ {<br>
+ simple_ptr<VB> vb(new VD());<br>
+ use(*vb);<br>
+ }<br>
+ {<br>
+ simple_ptr<VD> vd(new VD());<br>
+ use(*vd);<br>
+ }<br>
+ {<br>
+ simple_ptr<VF> vf(new VF());<br>
+ use(*vf);<br>
+ }<br>
+}<br>
+<br>
+void warn1() {<br>
+ {<br>
+ simple_ptr<B> b(new B()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr<dnvd::B>::~simple_ptr' requested here}}<br>
+ use(*b);<br>
+ }<br>
+ {<br>
+ simple_ptr2<B> b(new D()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr2<dnvd::B>::~simple_ptr2' requested here}}<br>
+ use(*b);<br>
+ }<br>
+ {<br>
+ simple_ptr<D> d(new D()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr<dnvd::D>::~simple_ptr' requested here}}<br>
+ use(*d);<br>
+ }<br>
+}<br>
+}<br>
+<br>
namespace PR9238 {<br>
class B { public: ~B(); };<br>
class C : virtual B { public: ~C() { } };<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>