Page MenuHomePhabricator

[libcxx] Add move constructor to compressed pair
AbandonedPublic

Authored by zoecarver on Oct 23 2019, 9:23 AM.

Details

Reviewers
ldionne
EricWF
mclow.lists
Group Reviewers
Restricted Project
Summary

This patch adds a move constructor to __compressed_pair so that move-only elements of nested compressed pairs (such as a move-only deleter in a std::shared_ptr) can be move-constructed.

Event Timeline

zoecarver created this revision.Oct 23 2019, 9:23 AM
zoecarver updated this revision to Diff 226153.Oct 23 2019, 9:24 AM
  • remove unnecessary template
Harbormaster completed remote builds in B39954: Diff 226153.
EricWF requested changes to this revision.Oct 24 2019, 4:52 AM

This test case already passes.

I don't see why this change is necessary.

This revision now requires changes to proceed.Oct 24 2019, 4:52 AM

I should have made it clear, my bad, this fixes the move constructor in C++03 mode.

I should have made it clear, my bad, this fixes the move constructor in C++03 mode.

I don't know how much we care to fix that. But if we do, does = default work instead?

I would have also expected = default to work but, it doesn't seem to. I think because __compressed_pair inherits from both its elements instead of storing them as members.

Yes, we care to fix that for D62274.

I would have also expected = default to work but, it doesn't seem to. I think because __compressed_pair inherits from both its elements instead of storing them as members.

You need to default it in both bases and in the compressed pair itself.

Yes, we care to fix that for D62274.

I don't think we care about move-only types in C++03.

Also, did you test this change? Because adding a move constructor to compressed pair suppresses the copy constructor and make the type always move only.

You need to default it in both bases and in the compressed pair itself.

Yes, you're right. That fixed it.

I don't think we care about move-only types in C++03.

Depends if we want to resolve 2802 in C++03. I think we do, if for no other reason than we don't want to have half a dozen more #ifndef blocks.

Also, did you test this change? Because adding a move constructor to compressed pair suppresses the copy constructor and make the type always move only.

It seemed to pass the memory tests. I also defaulted the copy constructor so, that shouldn't be an issue anymore.

zoecarver updated this revision to Diff 226278.Oct 24 2019, 9:27 AM
  • use = default in move and copy constructors
ldionne requested changes to this revision.Sep 14 2020, 1:24 PM

This sounds reasonable to me, however = default is not going to work in C++03 (unless there's a Clang extension I'm not aware of). @zoecarver Do you still need this?

This revision now requires changes to proceed.Sep 14 2020, 1:24 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 14 2020, 1:24 PM
zoecarver abandoned this revision.Sep 14 2020, 1:43 PM

I think I originally created this as a precursor to D62274. But, I don't think it's needed anymore.