[cfe-commits] r131989 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExprCXX.cpp test/SemaCXX/destructor.cpp
Argyrios Kyrtzidis
akyrtzi at gmail.com
Mon May 30 14:40:47 PDT 2011
On May 30, 2011, at 2:36 PM, Argyrios Kyrtzidis wrote:
> Hi Nico,
>
> On May 30, 2011, at 12:19 PM, Nico Weber wrote:
>
>> Hi Argyrios and Matthieu,
>>
>> 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.
>>
>> All of the false positives are of this form:
>>
>> class SomeInterface {
>> public:
>> virtual void interfaceMethod() {} // or = 0;
>> protected:
>> ~SomeInterface() {}
>> }
>>
>> class WorkerClass : public SomeInterface {
>> public:
>> // many non-virtual functions, but also:
>> virtual void interfaceMethod() override { /* do actual work */ }
>> };
>>
>> void f() {
>> scoped_ptr<WorkerClass> c(new WorkerClass); // simplified example
>> }
>>
>> This is a somewhat standard pattern (see e.g. http://www.gotw.ca/publications/mill18.htm, "Virtual Question #2").
>>
>> Do you have any good suggestions how to deal with this case?
>
> If WorkerClass gets subclassed in the future, deletion in "f()" will be undefined behavior. Ideally WorkerClass should be marked "final" and then there will also be no warning; does this sound reasonable ?
Um, to be exact, undefined behavior if you delete a WorkerClass * pointer.
>
> -Argyrios
>
>>
>> Thanks,
>> Nico
>>
>> On Tue, May 24, 2011 at 12:53 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> Author: akirtzidis
>> Date: Tue May 24 14:53:26 2011
>> New Revision: 131989
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=131989&view=rev
>> Log:
>> Add new warning that warns when invoking 'delete' on a polymorphic, non-final, class without a virtual destructor.
>>
>> Patch by Matthieu Monrocq!
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaExprCXX.cpp
>> cfe/trunk/test/SemaCXX/destructor.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=131989&r1=131988&r2=131989&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue May 24 14:53:26 2011
>> @@ -35,6 +35,8 @@
>> def : DiagGroup<"declaration-after-statement">;
>> def GNUDesignator : DiagGroup<"gnu-designator">;
>>
>> +def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor">;
>> +
>> def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
>> def DeprecatedWritableStr : DiagGroup<"deprecated-writable-strings">;
>> def Deprecated : DiagGroup<"deprecated", [ DeprecatedDeclarations] >,
>> @@ -235,6 +237,7 @@
>> def Most : DiagGroup<"most", [
>> CharSubscript,
>> Comment,
>> + DeleteNonVirtualDtor,
>> Format,
>> Implicit,
>> MismatchedTags,
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=131989&r1=131988&r2=131989&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue May 24 14:53:26 2011
>> @@ -3069,6 +3069,9 @@
>> def warn_non_virtual_dtor : Warning<
>> "%0 has virtual functions but non-virtual destructor">,
>> InGroup<NonVirtualDtor>, DefaultIgnore;
>> +def warn_delete_non_virtual_dtor : Warning<
>> + "delete called on %0 that has virtual functions but non-virtual destructor">,
>> + InGroup<DeleteNonVirtualDtor>, DefaultIgnore;
>> def warn_overloaded_virtual : Warning<
>> "%q0 hides overloaded virtual %select{function|functions}1">,
>> InGroup<OverloadedVirtual>, DefaultIgnore;
>>
>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=131989&r1=131988&r2=131989&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Tue May 24 14:53:26 2011
>> @@ -1800,6 +1800,20 @@
>> const_cast<CXXDestructorDecl*>(Dtor));
>> DiagnoseUseOfDecl(Dtor, StartLoc);
>> }
>> +
>> + // C++ [expr.delete]p3:
>> + // In the first alternative (delete object), if the static type of the
>> + // object to be deleted is different from its dynamic type, the static
>> + // type shall be a base class of the dynamic type of the object to be
>> + // deleted and the static type shall have a virtual destructor or the
>> + // behavior is undefined.
>> + //
>> + // Note: a final class cannot be derived from, no issue there
>> + if (!ArrayForm && RD->isPolymorphic() && !RD->hasAttr<FinalAttr>()) {
>> + CXXDestructorDecl *dtor = RD->getDestructor();
>> + if (!dtor || !dtor->isVirtual())
>> + Diag(StartLoc, diag::warn_delete_non_virtual_dtor) << PointeeElem;
>> + }
>> }
>>
>> if (!OperatorDelete) {
>>
>> Modified: cfe/trunk/test/SemaCXX/destructor.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/destructor.cpp?rev=131989&r1=131988&r2=131989&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/destructor.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/destructor.cpp Tue May 24 14:53:26 2011
>> @@ -1,4 +1,4 @@
>> -// RUN: %clang_cc1 -fsyntax-only -Wnon-virtual-dtor -verify %s
>> +// RUN: %clang_cc1 -std=c++0x -fsyntax-only -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -verify %s
>> class A {
>> public:
>> ~A();
>> @@ -173,6 +173,179 @@
>> TS2<int> foo; // expected-note {{instantiation}}
>> }
>>
>> +namespace dnvd { // delete-non-virtual-dtor warning
>> +struct NP {};
>> +
>> +struct B { // expected-warning {{has virtual functions but non-virtual destructor}}
>> + virtual void foo();
>> +};
>> +
>> +struct D: B {}; // expected-warning {{has virtual functions but non-virtual destructor}}
>> +
>> +struct F final: B {}; // expected-warning {{has virtual functions but non-virtual destructor}}
>> +
>> +struct VB {
>> + virtual void foo();
>> + virtual ~VB();
>> +};
>> +
>> +struct VD: VB {};
>> +
>> +struct VF final: VB {};
>> +
>> +template <typename T>
>> +class simple_ptr {
>> +public:
>> + simple_ptr(T* t): _ptr(t) {}
>> + ~simple_ptr() { delete _ptr; } // \
>> + // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}} \
>> + // expected-warning {{delete called on 'dnvd::D' that has virtual functions but non-virtual destructor}}
>> + T& operator*() const { return *_ptr; }
>> +private:
>> + T* _ptr;
>> +};
>> +
>> +template <typename T>
>> +class simple_ptr2 {
>> +public:
>> + simple_ptr2(T* t): _ptr(t) {}
>> + ~simple_ptr2() { delete _ptr; } // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}
>> + T& operator*() const { return *_ptr; }
>> +private:
>> + T* _ptr;
>> +};
>> +
>> +void use(B&);
>> +void use(VB&);
>> +
>> +void nowarnstack() {
>> + B b; use(b);
>> + D d; use(d);
>> + F f; use(f);
>> + VB vb; use(vb);
>> + VD vd; use(vd);
>> + VF vf; use(vf);
>> +}
>> +
>> +void nowarnnonpoly() {
>> + {
>> + NP* np = new NP();
>> + delete np;
>> + }
>> + {
>> + NP* np = new NP[4];
>> + delete[] np;
>> + }
>> +}
>> +
>> +void nowarnarray() {
>> + {
>> + B* b = new B[4];
>> + delete[] b;
>> + }
>> + {
>> + D* d = new D[4];
>> + delete[] d;
>> + }
>> + {
>> + VB* vb = new VB[4];
>> + delete[] vb;
>> + }
>> + {
>> + VD* vd = new VD[4];
>> + delete[] vd;
>> + }
>> +}
>> +
>> +template <typename T>
>> +void nowarntemplate() {
>> + {
>> + T* t = new T();
>> + delete t;
>> + }
>> + {
>> + T* t = new T[4];
>> + delete[] t;
>> + }
>> +}
>> +
>> +void nowarn0() {
>> + {
>> + F* f = new F();
>> + delete f;
>> + }
>> + {
>> + VB* vb = new VB();
>> + delete vb;
>> + }
>> + {
>> + VB* vb = new VD();
>> + delete vb;
>> + }
>> + {
>> + VD* vd = new VD();
>> + delete vd;
>> + }
>> + {
>> + VF* vf = new VF();
>> + delete vf;
>> + }
>> +}
>> +
>> +void warn0() {
>> + {
>> + B* b = new B();
>> + delete b; // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}
>> + }
>> + {
>> + B* b = new D();
>> + delete b; // expected-warning {{delete called on 'dnvd::B' that has virtual functions but non-virtual destructor}}
>> + }
>> + {
>> + D* d = new D();
>> + delete d; // expected-warning {{delete called on 'dnvd::D' that has virtual functions but non-virtual destructor}}
>> + }
>> +}
>> +
>> +void nowarn1() {
>> + {
>> + simple_ptr<F> f(new F());
>> + use(*f);
>> + }
>> + {
>> + simple_ptr<VB> vb(new VB());
>> + use(*vb);
>> + }
>> + {
>> + simple_ptr<VB> vb(new VD());
>> + use(*vb);
>> + }
>> + {
>> + simple_ptr<VD> vd(new VD());
>> + use(*vd);
>> + }
>> + {
>> + simple_ptr<VF> vf(new VF());
>> + use(*vf);
>> + }
>> +}
>> +
>> +void warn1() {
>> + {
>> + simple_ptr<B> b(new B()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr<dnvd::B>::~simple_ptr' requested here}}
>> + use(*b);
>> + }
>> + {
>> + simple_ptr2<B> b(new D()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr2<dnvd::B>::~simple_ptr2' requested here}}
>> + use(*b);
>> + }
>> + {
>> + simple_ptr<D> d(new D()); // expected-note {{in instantiation of member function 'dnvd::simple_ptr<dnvd::D>::~simple_ptr' requested here}}
>> + use(*d);
>> + }
>> +}
>> +}
>> +
>> namespace PR9238 {
>> class B { public: ~B(); };
>> class C : virtual B { public: ~C() { } };
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110530/5938c090/attachment.html>
More information about the cfe-commits
mailing list