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

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added a comment.

In D126194#3531280 <https://reviews.llvm.org/D126194#3531280>, @cor3ntin wrote:

> In D126194#3531267 <https://reviews.llvm.org/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 <type_traits>
  template<int N>
  struct A {
    A& operator=(A&&) requires true;
    virtual A& operator=(A&&);
  };
  static_assert(!std::is_aggregate_v<A<1>>);

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