This is an archive of the discontinued LLVM Phabricator instance.

shared_ptr deleter requirements (2802)
ClosedPublic

Authored by zoecarver on May 22 2019, 5:04 PM.

Details

Summary

This patch implements 2802. Requires _Deleter to have call operator and be move constructible. Based on D62233.

Refs PR37637.

Diff Detail

Event Timeline

zoecarver created this revision.May 22 2019, 5:04 PM
EricWF requested changes to this revision.May 22 2019, 11:21 PM
EricWF added a subscriber: EricWF.

This patch is missing the main point of LWG 2802, and that is making std::shared_ptr support move-only deleters. You need to make this test that constructors and make functions accepting a deleter accept this code:

template <class ValueT>
struct MyDeleter {
  MyDeleter() = delete;
  MyDeleter(MyDeleter const&) = delete;
  MyDeleter(MyDeleter&&) = default;

  explicit MyDeleter(secret_type) {} // so you can construct it for the test.

  void operator()(ValueT*); 
};

Additionally, the static asserts you're adding are required to be SFINAE checks according to the standard.
This was added by LWG 2875 (https://cplusplus.github.io/LWG/lwg-defects.html#2875)

include/memory
3719 ↗(On Diff #200839)

Does this compile in C++03, C++11, or C++14?

This revision now requires changes to proceed.May 22 2019, 11:21 PM
zoecarver updated this revision to Diff 201094.May 23 2019, 4:52 PM
  • fix static asserts
  • make compliant with C++11
  • fix 2875
  • update tests

I will fix the remaining C++03 issues in D62233.

zoecarver updated this revision to Diff 226156.Oct 23 2019, 9:38 AM
zoecarver edited the summary of this revision. (Show Details)

I rewrote this patch so that it doesn't require my (some, now obsolete) other patches. This patch does now rely on the fix from D69344 for C++03 move-constructibility. Also, I updated the tests.

Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2019, 9:38 AM
Herald added a subscriber: christof. · View Herald Transcript
zoecarver updated this revision to Diff 226158.Oct 23 2019, 9:41 AM

Don't mark 2875 as complete. That would requires some of my other patches so, I'll fix that issue down the road.

zoecarver marked 2 inline comments as done.Oct 23 2019, 9:42 AM
zoecarver added inline comments.
libcxx/include/memory
2201

Ignore this, it's part of D69344.

libcxx/test/libcxx/utilities/compressed_pair/move_const.pass.cpp
1 ↗(On Diff #226156)

Also part of D69344.

  • Rebase off master.
  • Dissable in C++03 mode.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 10 2020, 12:12 PM
zoecarver edited the summary of this revision. (Show Details)Jun 10 2020, 12:12 PM

In C++03 __compressed_pair doesn't have an implicit move constructor. So, I disabled the tests in C++03.

  • Mark as complete in cxx1z status
zoecarver edited the summary of this revision. (Show Details)Jun 10 2020, 12:15 PM
zoecarver edited the summary of this revision. (Show Details)
zoecarver marked an inline comment as done.
zoecarver added inline comments.
libcxx/www/cxx1z_status.html
461

The rest of the changes in this issue are addressed in D81414.

  • Rebase off main.
libcxx/include/memory
3971

I was just thinking, is this going to be problematic if __d has already been moved?

  • Update issue status.
ldionne requested changes to this revision.Feb 17 2021, 2:49 PM

LGTM except for the move issue (which is a really good catch BTW, I hadn't thought about that at all).

libcxx/include/memory
3971

Good observation. I think it's an issue, but it's really easy to fix. First, do we agree that line 3010 can't throw? Second, do we agree that we can assume move constructors don't throw?

So we only have to see whether something can throw on line 3009 after the deleter has been moved out of.

  • Clearly, copying or moving __p around isn't an issue, since it's a pointer.
  • Moving __d isn't an issue because we said move constructors don't throw.
  • Copying __a is potentially problematic, since it could throw. But if we use std::move(__a) instead, it's not an issue anymore.

None of the stuff inside the implementation of __shared_ptr_pointer::__shared_ptr_pointer is an issue either, since it just moves stuff into the compressed pairs, so the same logic applies.

This reasoning applies to all 4 constructors being touched by this patch. So I think we only need to use std::move(__a) and we're safe even from a very pedantic point of view.

This revision now requires changes to proceed.Feb 17 2021, 2:49 PM
zoecarver added inline comments.Feb 17 2021, 9:10 PM
libcxx/include/memory
3971

Hmm. I think move constructors shouldn't throw, but are allowed to. It's not undefined behavior to have a throwing move constructor (unless I'm mistaken, which is entirely possible). But I think you're right, the only thing we're really worried about is the allocator throwing. So what if we just put the call to .allocate inside of the try/catch? I'll update this patch to do that.

ldionne accepted this revision.Feb 18 2021, 7:53 AM
ldionne added inline comments.
libcxx/include/memory
3971

Yes, they are allowed to throw, but in that case we don't provide the strong exception guarantee, i.e. the code already behaves correctly. From cppreference:

To make the strong exception guarantee possible, user-defined move constructors should not throw exceptions. For example, std::vector relies on std::move_if_noexcept to choose between move and copy when the elements need to be relocated.

But anyway, scratch this discussion. I checked the requirements for this constructor, and it requires that constructing the deleter doesn't throw an exception. Furthermore, Allocators are always required to not throw exceptions when copied or moved. So there's no issue here, and the code is correct as-is. We can still use std::move(__a) for "efficiency", but there's not difference in correctness.

This revision now requires review to proceed.Feb 18 2021, 7:53 AM
ldionne removed a reviewer: EricWF.Feb 18 2021, 7:53 AM

Dropping Eric from reviewers so the change appears as accepted.

This revision is now accepted and ready to land.Feb 18 2021, 7:53 AM

Oops, and it does seem like this fails in C++03 mode - so please fix that C++03 failure but then you can commit. Thanks!

I'll fix the C++03 failures.

libcxx/include/memory
3971

OK. Thanks for checking :) I think using std::move on __a is a separate patch.

  • Fix build for C++03
  • std::move -> _VSTD::move
ldionne accepted this revision.Feb 18 2021, 1:43 PM
This revision was automatically updated to reflect the committed changes.