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@-1 7{{destructor declared here}}
~S() requires false;
- // expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-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@-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@-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.">;
+
}
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@-1 7{{destructor declared here}}
~S() requires false;
- // expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-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@-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@-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.">;
+
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
