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

[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<CXXDestructorDecl>(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