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

[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