[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:36:36 PDT 2011
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 ?
-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/cd848c20/attachment.html>
More information about the cfe-commits
mailing list