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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
