[cfe-commits] r131989 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExprCXX.cpp test/SemaCXX/destructor.cpp

[Nico Weber]

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?

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/0afc68e0/attachment.html>