Let -Wdelete-non-virtual-dtor fire in system headers too.

[thakis]

Summary

Makes the warning useful again in a std::unique_ptr world, PR28460.

Also make the warning not fire in unevaluated contexts, since system libraries (e.g. libc++) do do that. This would've been a good change before we started emitting this warning in system headers too, but "normal" code seems to be less template-heavy, so we didn't notice until now.

[thakis]

Don't warn in unevaluated contexts. Ready for a look now.

[dblaikie]

(if you'd prefer to wait for Reid or Richard to take a look, that's OK too :) )

Looks good to me, though wouldn't mind if the tests were a bit simpler - if they can be made so.

test/SemaCXX/destructor.cpp

3–28

Could this be simpler?

For example would it work to put this test at the end of the file, something like:

struct foo { virtual void f(); };
#pragma clang system_header
void f(foo *p) { delete p; }

[rsmith]

test/SemaCXX/destructor.cpp

27

Do we guarantee that __FILE__ names a path that can be used to include the current file? In other tests, we add -include %s to the RUN: line to model this situation.

[thakis]

Thanks, landed in r312167.

dblaikie: I believe the test can't be much simpler -- the pragma is file-based and having the main TU be marked as system header would be fairly different from what happens in real life even if it worked.

include/clang/Basic/DiagnosticSemaKinds.td

def warn_jump_out_of_seh_finally : Warning<
   "jump out of __finally block has undefined behavior">,
   InGroup<DiagGroup<"jump-seh-finally">>;
 def warn_non_virtual_dtor : Warning<
   "%0 has virtual functions but non-virtual destructor">,
   InGroup<NonVirtualDtor>, DefaultIgnore;
 def warn_delete_non_virtual_dtor : Warning<
   "%select{delete|destructor}0 called on non-final %1 that has "
   "virtual functions but non-virtual destructor">,
-  InGroup<DeleteNonVirtualDtor>, DefaultIgnore;
+  InGroup<DeleteNonVirtualDtor>, DefaultIgnore, ShowInSystemHeader;
 def note_delete_non_virtual : Note<
   "qualify call to silence this warning">;
 def warn_delete_abstract_non_virtual_dtor : Warning<
   "%select{delete|destructor}0 called on %1 that is abstract but has "
-  "non-virtual destructor">, InGroup<DeleteNonVirtualDtor>;
+  "non-virtual destructor">, InGroup<DeleteNonVirtualDtor>, ShowInSystemHeader;
 def warn_overloaded_virtual : Warning<
   "%q0 hides overloaded virtual %select{function|functions}1">,
   InGroup<OverloadedVirtual>, DefaultIgnore;
 def note_hidden_overloaded_virtual_declared_here : Note<
   "hidden overloaded virtual function %q0 declared here"
   "%select{|: different classes%diff{ ($ vs $)|}2,3"
   "|: different number of parameters (%2 vs %3)"
   "|: type mismatch at %ordinal2 parameter%diff{ ($ vs $)|}3,4"

lib/Sema/SemaExprCXX.cpp

Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
   AnalyzeDeleteExprMismatch(Result);
   return Result;
 }
 
 void Sema::CheckVirtualDtorCall(CXXDestructorDecl *dtor, SourceLocation Loc,
                                 bool IsDelete, bool CallCanBeVirtual,
                                 bool WarnOnNonAbstractTypes,
                                 SourceLocation DtorLoc) {
-  if (!dtor || dtor->isVirtual() || !CallCanBeVirtual)
+  if (!dtor || dtor->isVirtual() || !CallCanBeVirtual || isUnevaluatedContext())
     return;
 
   // 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.

test/SemaCXX/destructor.cpp

 // RUN: %clang_cc1 -std=c++11 -triple %itanium_abi_triple -fsyntax-only -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -fcxx-exceptions -verify %s
 // RUN: %clang_cc1 -std=c++11 -triple %ms_abi_triple -DMSABI -fsyntax-only -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -verify %s
+
+#if defined(BE_THE_HEADER)
+
+// Wdelete-non-virtual-dtor should warn about the delete from smart pointer
+// classes in system headers (std::unique_ptr...) too.
+
+#pragma clang system_header
+namespace dnvd {
+template <typename T>
+class simple_ptr {
+public:
+  simple_ptr(T* t): _ptr(t) {}
+  ~simple_ptr() { delete _ptr; } // \
+    // expected-warning {{delete called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} \
+    // expected-warning {{delete called on non-final 'dnvd::D' that has virtual functions but non-virtual destructor}}
+  T& operator*() const { return *_ptr; }
+private:
+  T* _ptr;
+};
+}
+
+#else
+
+#define BE_THE_HEADER
+#include __FILE__
+
 class A {
 public:
   ~A();
 };
 
 class B {
 public:
   ~B() { }
struct VB {
   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 non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} \
-    // expected-warning {{delete called on non-final '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 non-final 'dnvd::B' that has virtual functions but non-virtual destructor}}
   T& operator*() const { return *_ptr; }
 private:
   T* _ptr;
 };
void warn0() {
     delete b; // expected-warning {{delete called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}}
   }
   {
     D* d = new D();
     delete d; // expected-warning {{delete called on non-final 'dnvd::D' that has virtual functions but non-virtual destructor}}
   }
 }
 
+// Taken from libc++, slightly simplified.
+template <class>
+struct __is_destructible_apply { typedef int type; };
+struct __two {char __lx[2];};
+template <typename _Tp>
+struct __is_destructor_wellformed {
+  template <typename _Tp1>
+  static char __test(typename __is_destructible_apply<
+                       decltype(_Tp1().~_Tp1())>::type);
+  template <typename _Tp1>
+  static __two __test (...);
+              
+  static const bool value = sizeof(__test<_Tp>(12)) == sizeof(char);
+};
+
 void warn0_explicit_dtor(B* b, B& br, D* d) {
   b->~B(); // expected-warning {{destructor called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
   b->B::~B(); // No warning when the call isn't virtual.
 
+  // No warning in unevaluated contexts.
+  (void)__is_destructor_wellformed<B>::value;
+
   br.~B(); // expected-warning {{destructor called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
   br.B::~B();
 
   d->~D(); // expected-warning {{destructor called on non-final 'dnvd::D' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
   d->D::~D();
 }
 
 void nowarn1() {
void C1<T>::foo1() {
   C1::~C1();
 }
 
 void foo1() {
   C1<int> x;
   x.foo1();
 }
 }
+#endif // BE_THE_HEADER

test/SemaCXX/implicit-exception-spec.cpp

template<bool NE> struct D final : C<NE> {
     void f();
   };
 
   template<typename T, bool A, bool B, bool C, bool D, bool E, bool F> void check() {
     T *p = nullptr;
     T &a = *p;
     static_assert(noexcept(a = a) == D, "");
     static_assert(noexcept(a = static_cast<T&&>(a)) == E, "");
-    static_assert(noexcept(delete &a) == F, ""); // expected-warning 2{{abstract}}
+    static_assert(noexcept(delete &a) == F, "");
 
     // These are last because the first failure here causes instantiation to bail out.
     static_assert(noexcept(new (nothrow) T()) == A, ""); // expected-error 2{{abstract}}
     static_assert(noexcept(new (nothrow) T(a)) == B, "");
     static_assert(noexcept(new (nothrow) T(static_cast<T&&>(a))) == C, "");
   }
 
   template void check<A<false>, 0, 0, 0, 0, 0, 0>();