This is an archive of the discontinued LLVM Phabricator instance.

[Concepts] Implement overload resolution for destructors (P0848)
ClosedPublic

Authored by royjacobson on May 23 2022, 4:41 AM.

Details

Summary

This patch implements a necessary part of P0848, the overload resolution for destructors.
It is now possible to overload destructors based on constraints, and the eligible destructor
will be selected at the end of the class.

The approach this patch takes is to perform the overload resolution in Sema::ActOnFields
and to mark the selected destructor using a new property in FunctionDeclBitfields.

CXXRecordDecl::getDestructor is then modified to use this property to return the correct
destructor.

This closes https://github.com/llvm/llvm-project/issues/45614.

Diff Detail

Event Timeline

royjacobson created this revision.May 23 2022, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 4:41 AM
royjacobson requested review of this revision.May 23 2022, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 4:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
royjacobson edited the summary of this revision. (Show Details)May 23 2022, 4:42 AM
royjacobson added reviewers: Restricted Project, erichkeane, cor3ntin, aaron.ballman, rsmith.

How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?

clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

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.

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

clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

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

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.

clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

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.

erichkeane added inline comments.May 23 2022, 7:53 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

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).

royjacobson added inline comments.May 23 2022, 8:02 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

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)

erichkeane added inline comments.May 23 2022, 8:07 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
2330 ↗(On Diff #431333)

I'm pretty sure there is room in there. Updating the printers should be pretty easy as well.

Update the approach to use an AST property, and enable this for all language variants (not just CPP20).

There's still one test failing because OpenCL have got their own destructor thing going, I'll try to
see what I can do about it.

royjacobson edited the summary of this revision. (Show Details)May 23 2022, 10:14 PM
erichkeane added inline comments.May 24 2022, 6:49 AM
clang/lib/AST/DeclCXX.cpp
1901

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.

clang/lib/Sema/SemaDecl.cpp
17841

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.

royjacobson planned changes to this revision.May 30 2022, 4:21 AM
royjacobson added inline comments.
clang/lib/AST/DeclCXX.cpp
1901

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

clang/lib/Sema/SemaDecl.cpp
17841

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).

Update the PR to now handle triviality, which turned out to require a different approach.
On the bright side it is now easier to see how to implemenet the rest of P0848.

Added an AST dump test that checks the bitfields of structs and some derived traits.

Also fixed the OpenCL test error - their destructor model is quite incompatible with clang
and already broken in some ways, so I tried my best to continue the current behaviour.

I probably need to spend more time on this at one point, but first look seemed fine to me. I think the approach is about right, and the solution is there.

@aaron.ballman is messing with the constexpr-ness of SM functions, so there is likely some collaboration that needs to be doen to make sure we get THIS right as well.

clang/docs/ReleaseNotes.rst
403

Do we have enough to update cxx_status.html?

clang/include/clang/AST/DeclBase.h
1603

Hrumph... its a shame to lose the bit here later. I wonder if at one point in a future patch we should figure out which of these flags can be moved to an 'allocated in Trailing-storage ONLY if required' type deal, like we just did with FunctionTypeBitFields...

clang/include/clang/AST/DeclCXX.h
1430 ↗(On Diff #437656)

@aaron.ballman is messing around with addedMember recently... I wonder if there are OTHER decisions that need to be updated/moved here?

royjacobson added inline comments.Jun 17 2022, 1:06 AM
clang/docs/ReleaseNotes.rst
403

I think it's still partial enough that I don't want to communicate 'maybe it will work'.
I plan to finish the rest in time for Clang 15 so I prefer to just update it then :) But I don't feel very strongly about it either way.

clang/include/clang/AST/DeclCXX.h
1430 ↗(On Diff #437656)

Do you know what papers/DRs he's working on? It seemed like most constexpr stuff is done per-function and not per-class.

I don't think there's something else that needs to change but it's definitely possible I missed something.. I went through addedMember and tried to remove everything destructor related basically.

erichkeane accepted this revision.Jun 17 2022, 6:18 AM
erichkeane added inline comments.
clang/include/clang/AST/DeclCXX.h
1430 ↗(On Diff #437656)

He's working on DR647, which does a lot of work with special member functions/constexpr/consteval constructors.

That said, I believe he said it isn't a problem, they are related, but not conflicting in a way that he had.

This revision is now accepted and ready to land.Jun 17 2022, 6:18 AM
aaron.ballman added inline comments.Jun 17 2022, 6:38 AM
clang/include/clang/AST/DeclCXX.h
1430 ↗(On Diff #437656)

I don't see anything here that should conflict with what I'm working on (this is mostly about destructors and I've not started touching those yet). I'm working specifically on implementing this bit: http://eel.is/c++draft/dcl.constexpr#7

Fix the AST test on windows and rebase on HEAD.

This revision was landed with ongoing or failed builds.Jun 18 2022, 2:30 PM
This revision was automatically updated to reflect the committed changes.