[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson created this revision.
Herald added a project: All.
royjacobson requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We have received numeroud bug reports over P0848 not being implemented for destructors. We currently allow multiple destructor declarations but we will always select the first one, which might unintendedly compile with the wrong destructor. This patch adds a diagnostic for classes that try to overload destructors with constraints so users won't be confused when their (legal) code doesn't work.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126160

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/DeclCXX.cpp
  clang/test/CXX/over/over.match/over.match.viable/p3.cpp


Index: clang/test/CXX/over/over.match/over.match.viable/p3.cpp
===================================================================
--- clang/test/CXX/over/over.match/over.match.viable/p3.cpp
+++ clang/test/CXX/over/over.match/over.match.viable/p3.cpp
@@ -48,21 +48,20 @@
   void foo(A) requires true;
   S(A) requires false;
   S(double) requires true;
+  // expected-note at -1 7{{destructor declared here}}
   ~S() requires false;
-  // expected-note at -1 2{{because 'false' evaluated to false}}
+  // expected-note at -1 7{{destructor declared here}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
 };
 
 void bar() {
+  // Note - we have a hard error in this case until P0848 is implemented.
+  // expected-error@ 7{{overloading destructors is not supported yet}}
   WrapsStatics<int>::foo(A{});
   S<int>{1.}.foo(A{});
-  // expected-error at -1{{invalid reference to function '~S': constraints not satisfied}}
-  // Note - this behavior w.r.t. constrained dtors is a consequence of current
-  // wording, which does not invoke overload resolution when a dtor is called.
-  // P0848 is set to address this issue.
+
   S<int> s = 1;
-  // expected-error at -1{{invalid reference to function '~S': constraints not satisfied}}
   int a = s;
 }
Index: clang/lib/AST/DeclCXX.cpp
===================================================================
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -29,6 +29,7 @@
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticAST.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
@@ -1895,6 +1896,32 @@
 
   DeclContext::lookup_result R = lookup(Name);
 
+  // We don't support C++20 constrained destructors yet, and we are unlikely to
+  // do so in the near future. Until we do, we check here for multiple
+  // declarations and report an error. We have to handle correctly the case of
+  // merged declarations in modules and the case of invalid templated
+  // destructors so this is a bit involved.
+  if (!(R.empty() || R.isSingleResult())) {
+    bool hasNonTrivialOverloads = false;
+    auto firstDecl = R.front();
+    for (const auto &decl : R) {
+      if (decl->getFunctionType() && firstDecl->getFunctionType() &&
+          !Context.hasSameType(decl->getFunctionType(),
+                               firstDecl->getFunctionType())) {
+        hasNonTrivialOverloads = true;
+      }
+    }
+    if (hasNonTrivialOverloads) {
+      Context.getDiagnostics().Report(
+          diag::err_unsupported_destructor_overloading);
+      for (const auto &decl : R) {
+        Context.getDiagnostics().Report(
+            decl->getLocation(),
+            diag::note_unsupported_destructor_overloading_declaration);
+      }
+    }
+  }
+
   return R.empty() ? nullptr : dyn_cast<CXXDestructorDecl>(R.front());
 }
 
Index: clang/include/clang/Basic/DiagnosticASTKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticASTKinds.td
+++ clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -597,4 +597,11 @@
 def warn_unaligned_access : Warning<
   "field %1 within %0 is less aligned than %2 and is usually due to %0 being "
   "packed, which can lead to unaligned accesses">, InGroup<UnalignedAccess>, DefaultIgnore;
+
+def err_unsupported_destructor_overloading : Error<
+  "overloading destructors is not supported yet in this version. Consult "
+  "'https://github.com/llvm/llvm-project/issues/45614' for updates">;
+def note_unsupported_destructor_overloading_declaration : Note<
+  "destructor declared here.">;
+
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D126160.431221.patch
Type: text/x-patch
Size: 3720 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220522/a6e3c470/attachment.bin>