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

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

Reply via email to