Update noexcept behavior based on review feedback

Author: AaronBallman

clang/lib/Sema/SemaExceptionSpec.cpp

@@ -1200,21 +1200,35 @@ CanThrowResult Sema::canThrow(const Stmt *S) {
 
   case Expr::CXXDeleteExprClass: {
     auto *DE = cast<CXXDeleteExpr>(S);
-    CanThrowResult CT;
+    CanThrowResult CT = CT_Cannot;
     QualType DTy = DE->getDestroyedType();
     if (DTy.isNull() || DTy->isDependentType()) {
       CT = CT_Dependent;
     } else {
+      // C++20 [expr.delete]p6: If the value of the operand of the delete-
+      // expression is not a null pointer value and the selected deallocation
+      // function (see below) is not a destroying operator delete, the delete-
+      // expression will invoke the destructor (if any) for the object or the
+      // elements of the array being deleted.
+      //
+      // So if the destructor is virtual, we only look at the exception
+      // specification of the destructor regardless of what operator delete is
+      // selected. Otherwise, we look at the selected operator delete, and if
+      // it is not a destroying delete, then we look at the destructor.
       const FunctionDecl *OperatorDelete = DE->getOperatorDelete();
-      CT = canCalleeThrow(*this, DE, OperatorDelete);
-      if (!OperatorDelete->isDestroyingOperatorDelete()) {
-        if (const auto *RD = DTy->getAsCXXRecordDecl()) {
-          if (const CXXDestructorDecl *DD = RD->getDestructor())
-            CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, DD));
+      if (const auto *RD = DTy->getAsCXXRecordDecl()) {
+        if (const CXXDestructorDecl *DD = RD->getDestructor()) {
+          if (DD->isVirtual() || !OperatorDelete->isDestroyingOperatorDelete())
+            CT = canCalleeThrow(*this, DE, DD);
         }
-        if (CT == CT_Can)
-          return CT;
       }
+
+      // We always look at the exception specification of the operator delete.
+      CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, OperatorDelete));
+
+      // If we know we can throw, we're done.
+      if (CT == CT_Can)
+        return CT;
     }
     return mergeCanThrow(CT, canSubStmtsThrow(*this, DE));
   }

clang/test/SemaCXX/noexcept-destroying-delete.cpp

@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions -Wno-unevaluated-expression -std=c++20 %s
-// expected-no-diagnostics
 
 namespace std {
   struct destroying_delete_t {
@@ -31,3 +30,19 @@ ThrowingDestroyingDelete *pn = nullptr;
 // noexcept should return false here because the destroying delete itself is a
 // potentially throwing function.
 static_assert(!noexcept(delete(pn)));
+
+
+struct A {
+  virtual ~A(); // implicitly noexcept
+};
+struct B : A {
+  void operator delete(B *p, std::destroying_delete_t) { throw "oh no"; } // expected-warning {{'operator delete' has a non-throwing exception specification but can still throw}} \
+                                                                             expected-note {{deallocator has a implicit non-throwing exception specification}}
+};
+A *p = new B;
+
+// Because the destructor for A is virtual, it is the only thing we consider
+// when determining whether the delete expression can throw or not, despite the
+// fact that the selected operator delete is a destroying delete. For virtual
+// destructors, it's the dynamic type that matters.
+static_assert(noexcept(delete p));