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

Mon May 23 08:07:32 PDT 2022

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