[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-18 Thread Roy Jacobson via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG21eb1af469c3: [Concepts] Implement overload resolution for 
destructors (P0848) (authored by royjacobson).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/overloaded-destructors.cpp
  clang/test/CXX/class/class.dtor/p4.cpp
  clang/test/CXX/over/over.match/over.match.viable/p3.cpp
  clang/test/SemaTemplate/destructor-template.cpp

Index: clang/test/SemaTemplate/destructor-template.cpp
===
--- clang/test/SemaTemplate/destructor-template.cpp
+++ clang/test/SemaTemplate/destructor-template.cpp
@@ -98,7 +98,7 @@
   template 
   ~S(); // expected-error{{destructor cannot be declared as a template}}
 };
-struct T : S {// expected-note{{destructor of 'T' is implicitly deleted because base class 'PR38671::S' has no destructor}}
-  ~T() = default; // expected-warning{{explicitly defaulted destructor is implicitly deleted}}
+struct T : S {
+  ~T() = default;
 };
 } // namespace PR38671
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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete;  // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+// FIXME: We should probably make it illegal to mix virtual and non-virtual methods
+// this way. See CWG2488 and some discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105699.
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template 
+concept CO1 = N == 1;
+
+template 
+concept CO2 = N >
+0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 &) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires(T t) {
+  {t.foo()};
+};
+
+template 
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>; // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>; // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // 

[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-17 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson updated this revision to Diff 437940.
royjacobson added a comment.

Fix the AST test on windows and rebase on HEAD.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/overloaded-destructors.cpp
  clang/test/CXX/class/class.dtor/p4.cpp
  clang/test/CXX/over/over.match/over.match.viable/p3.cpp
  clang/test/SemaTemplate/destructor-template.cpp

Index: clang/test/SemaTemplate/destructor-template.cpp
===
--- clang/test/SemaTemplate/destructor-template.cpp
+++ clang/test/SemaTemplate/destructor-template.cpp
@@ -98,7 +98,7 @@
   template 
   ~S(); // expected-error{{destructor cannot be declared as a template}}
 };
-struct T : S {// expected-note{{destructor of 'T' is implicitly deleted because base class 'PR38671::S' has no destructor}}
-  ~T() = default; // expected-warning{{explicitly defaulted destructor is implicitly deleted}}
+struct T : S {
+  ~T() = default;
 };
 } // namespace PR38671
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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete;  // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+// FIXME: We should probably make it illegal to mix virtual and non-virtual methods
+// this way. See CWG2488 and some discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105699.
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template 
+concept CO1 = N == 1;
+
+template 
+concept CO2 = N >
+0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 &) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires(T t) {
+  {t.foo()};
+};
+
+template 
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>; // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>; // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // expected-error {{attempt to use a deleted function}}
+  A<2> a2; // expected-error {{attempt to use a deleted function}}
+  B<1> b1; 

[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-17 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:1430
+  /// out of which one will be selected at the end.
+  /// This is called separately from addedMember because it has to be deferred
+  /// to the completion of the class.

erichkeane wrote:
> royjacobson wrote:
> > erichkeane wrote:
> > > @aaron.ballman  is messing around with addedMember recently... I wonder 
> > > if there are OTHER decisions that need to be updated/moved here?
> > Do you know what papers/DRs he's working on? It seemed like most constexpr 
> > stuff is done per-function and not per-class.
> > 
> > I don't think there's something else that needs to change but it's 
> > definitely possible I missed something.. I went through addedMember and 
> > tried to remove everything destructor related basically.
> > 
> > 
> He's working on DR647, which does a lot of work with special member 
> functions/constexpr/consteval constructors.
> 
> That said, I believe he said it isn't a problem, they are related, but not 
> conflicting in a way that he had.
> 
> 
I don't see anything here that should conflict with what I'm working on (this 
is mostly about destructors and I've not started touching those yet). I'm 
working specifically on implementing this bit: 
http://eel.is/c++draft/dcl.constexpr#7


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-17 Thread Erich Keane via Phabricator via cfe-commits
erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/AST/DeclCXX.h:1430
+  /// out of which one will be selected at the end.
+  /// This is called separately from addedMember because it has to be deferred
+  /// to the completion of the class.

royjacobson wrote:
> erichkeane wrote:
> > @aaron.ballman  is messing around with addedMember recently... I wonder if 
> > there are OTHER decisions that need to be updated/moved here?
> Do you know what papers/DRs he's working on? It seemed like most constexpr 
> stuff is done per-function and not per-class.
> 
> I don't think there's something else that needs to change but it's definitely 
> possible I missed something.. I went through addedMember and tried to remove 
> everything destructor related basically.
> 
> 
He's working on DR647, which does a lot of work with special member 
functions/constexpr/consteval constructors.

That said, I believe he said it isn't a problem, they are related, but not 
conflicting in a way that he had.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-17 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:434
 
+- As per "Conditionally Trivial Special Member Functions" (P0848), it is
+  now possible to overload destructors using concepts. Note that the rest

erichkeane wrote:
> Do we have enough to update cxx_status.html?
I think it's still partial enough that I don't want to communicate 'maybe it 
will work'.
I plan to finish the rest in time for Clang 15 so I prefer to just update it 
then :) But I don't feel very strongly about it either way.




Comment at: clang/include/clang/AST/DeclCXX.h:1430
+  /// out of which one will be selected at the end.
+  /// This is called separately from addedMember because it has to be deferred
+  /// to the completion of the class.

erichkeane wrote:
> @aaron.ballman  is messing around with addedMember recently... I wonder if 
> there are OTHER decisions that need to be updated/moved here?
Do you know what papers/DRs he's working on? It seemed like most constexpr 
stuff is done per-function and not per-class.

I don't think there's something else that needs to change but it's definitely 
possible I missed something.. I went through addedMember and tried to remove 
everything destructor related basically.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-16 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment.

I probably need to spend more time on this at one point, but first look seemed 
fine to me.  I think the approach is about right, and the solution is there.

@aaron.ballman  is messing with the constexpr-ness of SM functions, so there is 
likely some collaboration that needs to be doen to make sure we get THIS right 
as well.




Comment at: clang/docs/ReleaseNotes.rst:434
 
+- As per "Conditionally Trivial Special Member Functions" (P0848), it is
+  now possible to overload destructors using concepts. Note that the rest

Do we have enough to update cxx_status.html?



Comment at: clang/include/clang/AST/DeclBase.h:1603
+/// [class.mem.special].
+uint64_t IsIneligibleOrNotSelected : 1;
+

Hrumph... its a shame to lose the bit here later.  I wonder if at one point in 
a future patch we should figure out which of these flags can be moved to an 
'allocated in Trailing-storage ONLY if required' type deal, like we just did 
with FunctionTypeBitFields...



Comment at: clang/include/clang/AST/DeclCXX.h:1430
+  /// out of which one will be selected at the end.
+  /// This is called separately from addedMember because it has to be deferred
+  /// to the completion of the class.

@aaron.ballman  is messing around with addedMember recently... I wonder if 
there are OTHER decisions that need to be updated/moved here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-06-16 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson updated this revision to Diff 437656.
royjacobson added a comment.

Update the PR to now handle triviality, which turned out to require a different 
approach.
On the bright side it is now easier to see how to implemenet the rest of P0848.

Added an AST dump test that checks the bitfields of structs and some derived 
traits.

Also fixed the OpenCL test error - their destructor model is quite incompatible 
with clang
and already broken in some ways, so I tried my best to continue the current 
behaviour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-decl.cpp
  clang/test/AST/overloaded-destructors.cpp
  clang/test/CXX/class/class.dtor/p4.cpp
  clang/test/CXX/over/over.match/over.match.viable/p3.cpp
  clang/test/SemaTemplate/destructor-template.cpp

Index: clang/test/SemaTemplate/destructor-template.cpp
===
--- clang/test/SemaTemplate/destructor-template.cpp
+++ clang/test/SemaTemplate/destructor-template.cpp
@@ -98,7 +98,7 @@
   template 
   ~S(); // expected-error{{destructor cannot be declared as a template}}
 };
-struct T : S {// expected-note{{destructor of 'T' is implicitly deleted because base class 'PR38671::S' has no destructor}}
-  ~T() = default; // expected-warning{{explicitly defaulted destructor is implicitly deleted}}
+struct T : S {
+  ~T() = default;
 };
 } // namespace PR38671
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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete;  // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+// FIXME: We should probably make it illegal to mix virtual and non-virtual methods
+// this way. See CWG2488 and some discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105699.
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template 
+concept CO1 = N == 1;
+
+template 
+concept CO2 = N >
+0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 &) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires(T t) {
+  {t.foo()};
+};
+
+template 
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>; // expected-error {{no viable destructor found for class 'D<0>'}} 

[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-30 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson planned changes to this revision.
royjacobson added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:1901
+  for (auto* Decl : R) {
+auto* DD = dyn_cast(Decl);
+if (DD && DD->isEligibleOrSelected())

erichkeane wrote:
> What cases happen when the lookup for a destructor name ends up not being a 
> destructor on a RecordDecl type?  I guess I could see this happening with a 
> pseudo-destructor, but that isn't a class type.
> 
> 
It's a bit silly, but it's because we can have template destructors that we 
added to the AST even though they're invalid (it ends up as a 
FunctionTemplateDecl).

I noticed it because of SemaTemplate/destructor-template.cpp



Comment at: clang/lib/Sema/SemaDecl.cpp:17841
+  if (CXXRecord && !CXXRecord->isDependentType())
+ComputeSelectedDestructor(*this, CXXRecord);
+

erichkeane wrote:
> How does all of this play with the 'defaulted destructor is constexpr' stuff? 
>  We end up storing facts like that, destructor 
> triviality/irrelevance/defaulted-and-constexpr/deleted/etc as a bit in the 
> Decl, rather than calculating it.  Can this feature (Allowing overloading) 
> cause those to be inaccurate?
> 
> That is, could we do something like use a requires clause to select between a 
> trivial, irrelevant, and constexpr destructor? Do we need to make sure we 
> update those too?  I would expect that the destructor here would need to 
> store its OWN trivaility/relevance/constexpr/etc here, so we can then update 
> those flags on the type once it is selected.
> 
You're right, I also found `canPassInRegisters` that will need to be adjusted a 
bit so it seems I need to write some codegen tests to make sure this gets the 
ABI correctly.

I won't have much time in the coming month, I hope to get to it by the start of 
July (but if someone wants to pick this up in the meantime feel free to do so).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-24 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:1901
+  for (auto* Decl : R) {
+auto* DD = dyn_cast(Decl);
+if (DD && DD->isEligibleOrSelected())

What cases happen when the lookup for a destructor name ends up not being a 
destructor on a RecordDecl type?  I guess I could see this happening with a 
pseudo-destructor, but that isn't a class type.





Comment at: clang/lib/Sema/SemaDecl.cpp:17841
+  if (CXXRecord && !CXXRecord->isDependentType())
+ComputeSelectedDestructor(*this, CXXRecord);
+

How does all of this play with the 'defaulted destructor is constexpr' stuff?  
We end up storing facts like that, destructor 
triviality/irrelevance/defaulted-and-constexpr/deleted/etc as a bit in the 
Decl, rather than calculating it.  Can this feature (Allowing overloading) 
cause those to be inaccurate?

That is, could we do something like use a requires clause to select between a 
trivial, irrelevant, and constexpr destructor? Do we need to make sure we 
update those too?  I would expect that the destructor here would need to store 
its OWN trivaility/relevance/constexpr/etc here, so we can then update those 
flags on the type once it is selected.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson updated this revision to Diff 431589.
royjacobson added a comment.

Update the approach to use an AST property, and enable this for all language 
variants (not just CPP20).

There's still one test failing because OpenCL have got their own destructor 
thing going, I'll try to
see what I can do about it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/class/class.dtor/p4.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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete; // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template
+concept CO1 = N == 1;
+
+template
+concept CO2 = N > 0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 && CO2) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires (T t) {
+  { t.foo() };
+};
+
+template
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>;  // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>;  // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // expected-error {{attempt to use a deleted function}}
+  A<2> a2; // expected-error {{attempt to use a deleted function}}
+  B<1> b1; // expected-error {{attempt to use a deleted function}}
+  B<2> b2; // expected-error {{attempt to use a deleted function}}
+  C<1> c1; // expected-error {{attempt to use a deleted function}}
+  C<2> c2; // expected-error {{attempt to use a deleted function}}
+  D<0> d0;
+  D<1> d1;
+  D<2> d2; // expected-error {{attempt to use a deleted function}}
+  E<1> e1; // expected-error {{attempt to use a deleted function}}
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -17755,6 +17755,62 @@
   AllIvarDecls.push_back(Ivar);
 }
 
+namespace {
+/// [class.dtor]p4:
+///   At the end of the definition of a class, overload resolution is
+///   performed among the prospective destructors declared in that class with
+///   an empty argument list to select the destructor for the class, also
+///   known as the selected destructor.
+///
+/// We do 

[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

royjacobson wrote:
> erichkeane wrote:
> > royjacobson wrote:
> > > cor3ntin wrote:
> > > > erichkeane wrote:
> > > > > I don't think this ends up being acceptable (removing them from the 
> > > > > AST).  Instead, we should probably mark them as "invalid" and update 
> > > > > getDestructor to only return the 'only' destructor, or the first 
> > > > > not-invalid one.
> > > > I'd rather we stay consistent with the wording, keep track of a 
> > > > selected destructor which would be returned by `getDestructor`. it's 
> > > > more surgery but it's a lot cleaner imo
> > > Corentin, do you suggest doing this in CXXRecordDecl explicitly?
> > > 
> > > Erich - I agree, this seems like a better solution, although this is a 
> > > bit of abuse to 'invalid' in the case of less-constrained candidates. 
> > > Ideally we might want another AST property of 'eligible' and keep track 
> > > of things there, but I don't really know the AST code well enough to do 
> > > it.
> > I like the idea of marking them eligible that way.  If this JUST has to do 
> > with Destructors for now, adding a bit to `CXXDestructorDecl` is a pretty 
> > trivial thing to do.  I'm pretty sure it is defined in `DeclCXX.h`. At that 
> > point, we just need to make sure it is checked during 'getDestructor' (or 
> > at least, around any calls to that).
> I think we'll need it soon for the other SMF as well. (Or maybe even for all 
> member functions if CWG2421 is ever resolved). 
> 
> So maybe I should just take some time to look at `FunctionDeclBitfields` and 
> see if I can update it. (I hope updating the ast printers isn't too hard)
I'm pretty sure there is room in there.  Updating the printers should be pretty 
easy as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

erichkeane wrote:
> royjacobson wrote:
> > cor3ntin wrote:
> > > erichkeane wrote:
> > > > I don't think this ends up being acceptable (removing them from the 
> > > > AST).  Instead, we should probably mark them as "invalid" and update 
> > > > getDestructor to only return the 'only' destructor, or the first 
> > > > not-invalid one.
> > > I'd rather we stay consistent with the wording, keep track of a selected 
> > > destructor which would be returned by `getDestructor`. it's more surgery 
> > > but it's a lot cleaner imo
> > Corentin, do you suggest doing this in CXXRecordDecl explicitly?
> > 
> > Erich - I agree, this seems like a better solution, although this is a bit 
> > of abuse to 'invalid' in the case of less-constrained candidates. Ideally 
> > we might want another AST property of 'eligible' and keep track of things 
> > there, but I don't really know the AST code well enough to do it.
> I like the idea of marking them eligible that way.  If this JUST has to do 
> with Destructors for now, adding a bit to `CXXDestructorDecl` is a pretty 
> trivial thing to do.  I'm pretty sure it is defined in `DeclCXX.h`. At that 
> point, we just need to make sure it is checked during 'getDestructor' (or at 
> least, around any calls to that).
I think we'll need it soon for the other SMF as well. (Or maybe even for all 
member functions if CWG2421 is ever resolved). 

So maybe I should just take some time to look at `FunctionDeclBitfields` and 
see if I can update it. (I hope updating the ast printers isn't too hard)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

royjacobson wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > I don't think this ends up being acceptable (removing them from the AST). 
> > >  Instead, we should probably mark them as "invalid" and update 
> > > getDestructor to only return the 'only' destructor, or the first 
> > > not-invalid one.
> > I'd rather we stay consistent with the wording, keep track of a selected 
> > destructor which would be returned by `getDestructor`. it's more surgery 
> > but it's a lot cleaner imo
> Corentin, do you suggest doing this in CXXRecordDecl explicitly?
> 
> Erich - I agree, this seems like a better solution, although this is a bit of 
> abuse to 'invalid' in the case of less-constrained candidates. Ideally we 
> might want another AST property of 'eligible' and keep track of things there, 
> but I don't really know the AST code well enough to do it.
I like the idea of marking them eligible that way.  If this JUST has to do with 
Destructors for now, adding a bit to `CXXDestructorDecl` is a pretty trivial 
thing to do.  I'm pretty sure it is defined in `DeclCXX.h`. At that point, we 
just need to make sure it is checked during 'getDestructor' (or at least, 
around any calls to that).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson added a comment.

In D126194#3531280 , @cor3ntin wrote:

> In D126194#3531267 , @erichkeane 
> wrote:
>
>> How much of P0848 is missing after this?  If nothing/not much, should we 
>> update cxx_status.html as well?
>
> P0848 applies to all special member function. At best we could mark it 
> partial but most of the work still need to be done.
> I gave a shot to P0848 a few months ago, but my assesment is that clang might 
> have to significantly refactor of special member functions to do that cleanly

Corentin, are there other places like getDestructor where we need the 
constraints Sema information in the AST? I hoped the other special member 
functions go through LookupSpecialMember which does the needed overload 
resolution.

So as far as I understand the code base, the P0848 part that remains is 
computing the SMFs that are 'eligible' and update the type traits 
(trivial/trivially copyable) accordingly. Currently we mark those inside 
CXXRecordDecl::addedMember, so we'll probably have to override them. I also 
don't understand how exactly the eligibility checks interact with the deferred 
concept checking.

BTW, this also interacts funnily with other type traits. For example, this is 
apparently legal

  #include 
  template
  struct A {
A& operator=(A&&) requires true;
virtual A& operator=(A&&);
  };
  static_assert(!std::is_aggregate_v>);

So ineligible SMFs are ineligible only for the purpose of [copy]triviality, and 
can still have other visible effects!

About the status page - we're going to break ABI when we implement the type 
traits change so I don't think we should update it yet.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

cor3ntin wrote:
> erichkeane wrote:
> > I don't think this ends up being acceptable (removing them from the AST).  
> > Instead, we should probably mark them as "invalid" and update getDestructor 
> > to only return the 'only' destructor, or the first not-invalid one.
> I'd rather we stay consistent with the wording, keep track of a selected 
> destructor which would be returned by `getDestructor`. it's more surgery but 
> it's a lot cleaner imo
Corentin, do you suggest doing this in CXXRecordDecl explicitly?

Erich - I agree, this seems like a better solution, although this is a bit of 
abuse to 'invalid' in the case of less-constrained candidates. Ideally we might 
want another AST property of 'eligible' and keep track of things there, but I 
don't really know the AST code well enough to do it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment.

In D126194#3531267 , @erichkeane 
wrote:

> How much of P0848 is missing after this?  If nothing/not much, should we 
> update cxx_status.html as well?

P0848 applies to all special member function. At best we could mark it partial 
but most of the work still need to be done.
I gave a shot to P0848 a few months ago, but my assesment is that clang might 
have to significantly refactor of special member functions to do that cleanly




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

erichkeane wrote:
> I don't think this ends up being acceptable (removing them from the AST).  
> Instead, we should probably mark them as "invalid" and update getDestructor 
> to only return the 'only' destructor, or the first not-invalid one.
I'd rather we stay consistent with the wording, keep track of a selected 
destructor which would be returned by `getDestructor`. it's more surgery but 
it's a lot cleaner imo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment.

How much of P0848 is missing after this?  If nothing/not much, should we update 
cxx_status.html as well?




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2330
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and 
rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of

I don't think this ends up being acceptable (removing them from the AST).  
Instead, we should probably mark them as "invalid" and update getDestructor to 
only return the 'only' destructor, or the first not-invalid one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread Roy Jacobson via Phabricator via cfe-commits
royjacobson updated this revision to Diff 431333.
royjacobson added a comment.

Fix typo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126194/new/

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/class/class.dtor/p4.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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete; // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template
+concept CO1 = N == 1;
+
+template
+concept CO2 = N > 0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 && CO2) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires (T t) {
+  { t.foo() };
+};
+
+template
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>;  // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>;  // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // expected-error {{attempt to use a deleted function}}
+  A<2> a2; // expected-error {{attempt to use a deleted function}}
+  B<1> b1; // expected-error {{attempt to use a deleted function}}
+  B<2> b2; // expected-error {{attempt to use a deleted function}}
+  C<1> c1; // expected-error {{attempt to use a deleted function}}
+  C<2> c2; // expected-error {{attempt to use a deleted function}}
+  D<0> d0;
+  D<1> d1;
+  D<2> d2; // expected-error {{attempt to use a deleted function}}
+  E<1> e1; // expected-error {{attempt to use a deleted function}}
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2320,6 +2320,81 @@
 }
   };
 
+  /// [class.dtor]p4:
+  ///   At the end of the definition of a class, overload resolution is
+  ///   performed among the prospective destructors declared in that class with
+  ///   an empty argument list to select the destructor for the class, also
+  ///   known as the selected destructor.
+  ///
+  /// To achieve that, we remove all non-selected destructors from the AST,
+  /// which is a bit unusual. We can't let those declarations be in AST and rely
+  /// on LookupSpecialMember to return the correct declaration because a lot of
+  /// code relies on CXXRecordDecl::getDestructor() which assumes there can only
+  /// 

[PATCH] D126194: [Concepts] Implement overload resolution for destructors (P0848)

2022-05-23 Thread 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.

This patch implements a necessary part of P0848, the overload resolution for 
destructors.
It is now possible to overload destructors based on constraints, and the 
eligible destructor
will be selected at the end of the class.

The approach this patch takes is to perform the overload resolution after 
instantiating the members
in Sema::InstantiateClass, and to remove from the CXXRecordDecl all 
declarations of non-selected
destructors.

This is the approach that I found that can satisfy the following requirements:

1. The overload resolution and constraint checking is performed after the other 
members are defined but before the type is complete. This is important because 
constraints can check other members of the class, but the result of the 
overload resolution can change the type traits (triviallity etc.) of the class.
2. The order of (visible) functions in the AST is kept. This is important 
because it affects ABI through the order of functions in the vtable.

Note: this is only enabled for CPP20 - I tested it without the CPP20 
restriction and
all the tests passed, so I can remove the restriction. I slightly prefer to 
remove it because
it will be easier to get coverage for this and it seems safer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126194

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/CXX/class/class.dtor/p4.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
@@ -49,7 +49,6 @@
   S(A) requires false;
   S(double) requires true;
   ~S() requires false;
-  // expected-note@-1 2{{because 'false' evaluated to false}}
   ~S() requires true;
   operator int() requires true;
   operator int() requires false;
@@ -58,11 +57,7 @@
 void bar() {
   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/test/CXX/class/class.dtor/p4.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.dtor/p4.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+struct A {
+  ~A() = delete; // expected-note {{explicitly marked deleted}}
+  ~A() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct B {
+  ~B() requires(N == 1) = delete; // expected-note {{explicitly marked deleted}}
+  virtual ~B() = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template
+concept CO1 = N == 1;
+
+template
+concept CO2 = N > 0;
+
+template 
+struct C {
+  ~C() = delete; // expected-note {{explicitly marked deleted}}
+  ~C() requires(CO1) = delete;
+  ~C() requires(CO1 && CO2) = delete; // expected-note {{explicitly marked deleted}}
+};
+
+template 
+struct D {
+  ~D() requires(N != 0) = delete; // expected-note {{explicitly marked deleted}}
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+  ~D() requires(N == 1) = delete;
+  // expected-note@-1 {{candidate function has been explicitly deleted}}
+  // expected-note@-2 {{candidate function not viable: constraints not satisfied}}
+  // expected-note@-3 {{evaluated to false}}
+};
+
+template 
+concept Foo = requires (T t) {
+  { t.foo() };
+};
+
+template
+struct E {
+  void foo();
+  ~E();
+  ~E() requires Foo = delete;  // expected-note {{explicitly marked deleted}}
+};
+
+template struct A<1>;
+template struct A<2>;
+template struct B<1>;
+template struct B<2>;
+template struct C<1>;
+template struct C<2>;
+template struct D<0>;  // expected-error {{no viable destructor found for class 'D<0>'}} expected-note {{in instantiation of template}}
+template struct D<1>;  // expected-error {{destructor of class 'D<1>' is ambiguous}} expected-note {{in instantiation of template}}
+template struct D<2>;
+template struct E<1>;
+
+int main() {
+  A<1> a1; // expected-error {{attempt to use a deleted function}}
+  A<2> a2; // expected-error {{attempt to use a deleted function}}
+  B<1> b1; // expected-error {{attempt to use a deleted