[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
royjacobson abandoned this revision. royjacobson added a comment. I continued hacking a bit at this and I think I got a working approach to implementing P0848 (down to 6 failing tests, will probably post at least a draft tomorrow). So I'll close for now. Maybe it's still useful for backporting if someone wants to. Sorry for all the mail spam from today... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126160/new/ https://reviews.llvm.org/D126160 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
royjacobson updated this revision to Diff 431263.
royjacobson added a comment.
Fix conventions
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126160/new/
https://reviews.llvm.org/D126160
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDeclCXX.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
@@ -44,24 +44,26 @@
template
struct S {
+ // expected-error@-1 {{overloading destructors is not supported}}
void foo(int) requires false;
void foo(A) requires true;
S(A) requires false;
S(double) requires true;
~S() requires false;
// expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-2 {{destructor declared here}}
~S() requires true;
+ // expected-note@-1 {{destructor declared here}}
operator int() requires true;
operator int() requires false;
};
void bar() {
+ // Note - we have a hard error in this case until P0848 is implemented.
WrapsStatics::foo(A{});
S{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 s = 1;
// expected-error@-1{{invalid reference to function '~S': constraints not satisfied}}
int a = s;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6731,6 +6731,45 @@
return IssuedDiagnostic;
}
+namespace {
+
+/// 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.
+/// We also don't emit errors on OpenCL code because OpenCL destructors can
+/// overload on address space (https://reviews.llvm.org/D64569).
+void DiagnoseUnsupportedDestructorOverloading(Sema& S, const CXXRecordDecl* Record) {
+ ASTContext = Record->getASTContext();
+ QualType ClassType = Context.getTypeDeclType(Record);
+
+ DeclarationName Name = Context.DeclarationNames.getCXXDestructorName(
+ Context.getCanonicalType(ClassType));
+
+ DeclContext::lookup_result R = Record->lookup(Name);
+
+ if (!(R.empty() || R.isSingleResult()) && !Context.getLangOpts().OpenCL) {
+bool HasNonTrivialOverloads = false;
+auto FirstDecl = R.front();
+for (const auto : R) {
+ if (!Decl->isTemplateDecl() && !FirstDecl->isTemplateDecl() &&
+ !Context.isSameEntity(Decl, FirstDecl)) {
+HasNonTrivialOverloads = true;
+ }
+}
+if (HasNonTrivialOverloads) {
+ S.Diag(Record->getLocation(),
+ diag::err_unsupported_destructor_overloading);
+ for (const auto : R) {
+S.Diag(Decl->getLocation(),
+ diag::note_unsupported_destructor_overloading_declaration);
+ }
+}
+ }
+}
+} // namespace
+
/// Perform semantic checks on a class definition that has been
/// completing, introducing implicitly-declared members, checking for
/// abstract types, etc.
@@ -6955,6 +6994,9 @@
CheckCompletedMemberFunction(M);
};
+ if (!inTemplateInstantiation())
+DiagnoseUnsupportedDestructorOverloading(*this, Record);
+
// Check the destructor before any other member function. We need to
// determine whether it's trivial in order to determine whether the claas
// type is a literal type, which is a prerequisite for determining whether
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2892,6 +2892,11 @@
def err_unsupported_placeholder_constraint : Error<
"constrained placeholder types other than simple 'auto' on non-type template "
"parameters not supported yet">;
+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.">;
def err_template_different_requires_clause : Error<
"requires clause differs in template redeclaration">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
royjacobson updated this revision to Diff 431251.
royjacobson added a comment.
Move diagnostic into Sema, make it fire once for every class template
definitions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126160/new/
https://reviews.llvm.org/D126160
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDeclCXX.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
@@ -44,24 +44,26 @@
template
struct S {
+ // expected-error@-1 {{overloading destructors is not supported}}
void foo(int) requires false;
void foo(A) requires true;
S(A) requires false;
S(double) requires true;
~S() requires false;
// expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-2 {{destructor declared here}}
~S() requires true;
+ // expected-note@-1 {{destructor declared here}}
operator int() requires true;
operator int() requires false;
};
void bar() {
+ // Note - we have a hard error in this case until P0848 is implemented.
WrapsStatics::foo(A{});
S{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 s = 1;
// expected-error@-1{{invalid reference to function '~S': constraints not satisfied}}
int a = s;
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6731,6 +6731,45 @@
return IssuedDiagnostic;
}
+namespace {
+
+/// 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.
+/// We also don't emit errors on OpenCL code because OpenCL destructors can
+/// overload on address space (https://reviews.llvm.org/D64569).
+void DiagnoseUnsupportedDestructorOverloading(Sema& S, const CXXRecordDecl* Record) {
+ ASTContext = Record->getASTContext();
+ QualType ClassType = Context.getTypeDeclType(Record);
+
+ DeclarationName Name = Context.DeclarationNames.getCXXDestructorName(
+ Context.getCanonicalType(ClassType));
+
+ DeclContext::lookup_result R = Record->lookup(Name);
+
+ if (!(R.empty() || R.isSingleResult()) && !Context.getLangOpts().OpenCL) {
+bool hasNonTrivialOverloads = false;
+auto firstDecl = R.front();
+for (const auto : R) {
+ if (!decl->isTemplateDecl() && !firstDecl->isTemplateDecl() &&
+ !Context.isSameEntity(decl, firstDecl)) {
+hasNonTrivialOverloads = true;
+ }
+}
+if (hasNonTrivialOverloads) {
+ S.Diag(Record->getLocation(),
+ diag::err_unsupported_destructor_overloading);
+ for (const auto : R) {
+S.Diag(decl->getLocation(),
+ diag::note_unsupported_destructor_overloading_declaration);
+ }
+}
+ }
+}
+} // namespace
+
/// Perform semantic checks on a class definition that has been
/// completing, introducing implicitly-declared members, checking for
/// abstract types, etc.
@@ -6955,6 +6994,9 @@
CheckCompletedMemberFunction(M);
};
+ if (!inTemplateInstantiation())
+DiagnoseUnsupportedDestructorOverloading(*this, Record);
+
// Check the destructor before any other member function. We need to
// determine whether it's trivial in order to determine whether the claas
// type is a literal type, which is a prerequisite for determining whether
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2892,6 +2892,11 @@
def err_unsupported_placeholder_constraint : Error<
"constrained placeholder types other than simple 'auto' on non-type template "
"parameters not supported yet">;
+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.">;
def err_template_different_requires_clause : Error<
"requires clause differs in template redeclaration">;
___
cfe-commits
[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
royjacobson planned changes to this revision. royjacobson added a comment. I've thought about this a bit more and I want to move the diagnostic into SemaTemplateInstantiateDecl or somewhere close. It makes more sense because it's closer to where P0848 needs to be implemented and it will spam less errors. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126160/new/ https://reviews.llvm.org/D126160 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
royjacobson updated this revision to Diff 431229.
royjacobson added a comment.
A working version.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126160/new/
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
@@ -44,24 +44,26 @@
template
struct S {
+ // expected-error@-1 7{{overloading destructors is not supported}}
void foo(int) requires false;
void foo(A) requires true;
S(A) requires false;
S(double) requires true;
~S() requires false;
// expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-2 7{{destructor declared here}}
~S() requires true;
+ // expected-note@-1 7{{destructor declared here}}
operator int() requires true;
operator int() requires false;
};
void bar() {
+ // Note - we have a hard error in this case until P0848 is implemented.
WrapsStatics::foo(A{});
S{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 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,33 @@
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.
+ // We also don't emit errors on OpenCL code because OpenCL desturctors can
+ // overload on address space (https://reviews.llvm.org/D64569).
+ if (!(R.empty() || R.isSingleResult()) && !Context.getLangOpts().OpenCL) {
+bool hasNonTrivialOverloads = false;
+auto firstDecl = R.front();
+for (const auto : R) {
+ if (!decl->isTemplateDecl() && !firstDecl->isTemplateDecl() &&
+ !Context.isSameEntity(decl, firstDecl)) {
+hasNonTrivialOverloads = true;
+ }
+}
+if (hasNonTrivialOverloads) {
+ Context.getDiagnostics().Report(
+ getLocation(), diag::err_unsupported_destructor_overloading);
+ for (const auto : R) {
+Context.getDiagnostics().Report(
+decl->getLocation(),
+diag::note_unsupported_destructor_overloading_declaration);
+ }
+}
+ }
+
return R.empty() ? nullptr : dyn_cast(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,
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
@@ -44,24 +44,26 @@
template
struct S {
+ // expected-error@-1 7{{overloading destructors is not supported}}
void foo(int) requires false;
void foo(A) requires true;
S(A) requires false;
S(double) requires true;
~S() requires false;
// expected-note@-1 2{{because 'false' evaluated to false}}
+ // expected-note@-2 7{{destructor declared here}}
~S() requires true;
+ // expected-note@-1 7{{destructor declared here}}
operator int() requires true;
operator int() requires false;
};
[PATCH] D126160: [Concepts] Add an error for unsupported P0848 (destructor overloading) code
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::foo(A{});
S{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 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 : 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 : R) {
+Context.getDiagnostics().Report(
+decl->getLocation(),
+diag::note_unsupported_destructor_overloading_declaration);
+ }
+}
+ }
+
return R.empty() ? nullptr : dyn_cast(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,
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
