Update based on review feedback

Author: AaronBallman

clang/include/clang/AST/DeclCXX.h

@@ -2855,6 +2855,11 @@ class CXXDestructorDecl : public CXXMethodDecl {
     return getCanonicalDecl()->OperatorDeleteThisArg;
   }
 
+  /// Will this destructor ever be called when considering which deallocation
+  /// function is associated with the destructor? Can optionally be passed an
+  /// 'operator delete' function declaration to test against specifically.
+  bool isDestructorCalled(const FunctionDecl *OpDel = nullptr) const;
+
   CXXDestructorDecl *getCanonicalDecl() override {
     return cast<CXXDestructorDecl>(FunctionDecl::getCanonicalDecl());
   }

clang/lib/AST/DeclCXX.cpp

@@ -2969,6 +2969,28 @@ void CXXDestructorDecl::setOperatorDelete(FunctionDecl *OD, Expr *ThisArg) {
   }
 }
 
+bool CXXDestructorDecl::isDestructorCalled(const FunctionDecl *OpDel) const {
+  // 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.
+  //
+  // This means we should not look at the destructor for a destroying
+  // delete operator, as that destructor is never called, unless the
+  // destructor is virtual (see [expr.delete]p8.1) because then the
+  // selected operator depends on the dynamic type of the pointer.
+  const FunctionDecl *SelectedOperatorDelete = OpDel ? OpDel : OperatorDelete;
+  if (!SelectedOperatorDelete)
+    return true;
+
+  if (!SelectedOperatorDelete->isDestroyingOperatorDelete())
+    return true;
+
+  // We have a destroying operator delete, so it depends on the dtor.
+  return isVirtual();
+}
+
 void CXXConversionDecl::anchor() {}
 
 CXXConversionDecl *CXXConversionDecl::CreateDeserialized(ASTContext &C,

clang/lib/Sema/SemaExceptionSpec.cpp

@@ -1210,17 +1210,11 @@ CanThrowResult Sema::canThrow(const Stmt *S) {
       // 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();
       if (const auto *RD = DTy->getAsCXXRecordDecl()) {
-        if (const CXXDestructorDecl *DD = RD->getDestructor()) {
-          if (DD->isVirtual() || !OperatorDelete->isDestroyingOperatorDelete())
-            CT = canCalleeThrow(*this, DE, DD);
-        }
+        if (const CXXDestructorDecl *DD = RD->getDestructor();
+            DD && DD->isDestructorCalled(OperatorDelete))
+          CT = canCalleeThrow(*this, DE, DD);
       }
 
       // We always look at the exception specification of the operator delete.

clang/lib/Sema/SemaExprCXX.cpp

@@ -3767,29 +3767,6 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
 
     DeclarationName DeleteName = Context.DeclarationNames.getCXXOperatorName(
                                       ArrayForm ? OO_Array_Delete : OO_Delete);
-
-    // 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.
-    //
-    // This means we should not look at the destructor for a destroying
-    // delete operator, as that destructor is never called, unless the
-    // destructor is virtual (see [expr.delete]p8.1) because then the
-    // selected operator depends on the dynamic type of the pointer.
-    auto IsDtorCalled = [](const CXXMethodDecl *Dtor,
-                           const FunctionDecl *OperatorDelete) {
-      if (!OperatorDelete)
-        return true;
-
-      if (!OperatorDelete->isDestroyingOperatorDelete())
-        return true;
-
-      // We have a destroying operator delete, so it depends on the dtor.
-      return Dtor->isVirtual();
-    };
-
     if (PointeeRD) {
       if (!UseGlobal &&
           FindDeallocationFunction(StartLoc, PointeeRD, DeleteName,
@@ -3816,7 +3793,7 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
 
       if (!PointeeRD->hasIrrelevantDestructor()) {
         if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
-          if (IsDtorCalled(Dtor, OperatorDelete)) {
+          if (Dtor->isDestructorCalled(OperatorDelete)) {
             MarkFunctionReferenced(StartLoc,
                                    const_cast<CXXDestructorDecl *>(Dtor));
             if (DiagnoseUseOfDecl(Dtor, StartLoc))
@@ -3858,7 +3835,7 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
     bool IsVirtualDelete = false;
     if (PointeeRD) {
       if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
-        if (IsDtorCalled(Dtor, OperatorDelete))
+        if (Dtor->isDestructorCalled(OperatorDelete))
           CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
                                 PDiag(diag::err_access_dtor) << PointeeElem);
         IsVirtualDelete = Dtor->isVirtual();