Addresses LWG 3548 which mandates that when shared_ptr is being constructed from a unique_ptr, the unique_ptr's deleter should be moved and not copied.
Details
Details
- Reviewers
philnik • Quuxplusone EricWF - Group Reviewers
Restricted Project - Commits
- rG34538dba9be6: [libc++] Make shared_ptr move unique_ptr's deleter
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
| libcxx/include/__memory/shared_ptr.h | ||
|---|---|---|
| 656 | Should the requirement be added here, or should we reuse __shared_ptr_deleter_ctor_reqs? libstdc++ has a similar alias template used here that sfinaes away without move_constructible. That might be more readable, even though the alias template would only be used here iiuc. Also, would it be nicer to use _And? | |
| libcxx/include/__memory/shared_ptr.h | ||
|---|---|---|
| 656 | I think it's OK the way it is. Don't use _And if there is no good reason. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
| 87 | Could you = delete the default constructor? That ensures the deleter is actually moved properly. Also count how many moves have been made and ensure it's exactly one. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 87 | unique_ptr's deleter is constrained by _EnableIfDeleterDefaultConstructible so I can't do that iiuc. Would it suffice to have counts for move and default constructors, and assert that each constructor was called the expected number of times? | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 87 | The constructors where the deleter has to be default-constructible are disabled, but not unique_ptr itself. The unique_ptr(nullptr_t, deleter_type&&) and unique_ptr(pointer, deleter_type&&) constructors are still available. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 87 | I see, thanks! | |
Comment Actions
Thanks for working on this! You should also git grep 3548 ../libcxx and address anything you find there. (The result set will be non-empty.)
| libcxx/include/__memory/shared_ptr.h | ||
|---|---|---|
| 656 | I think the constraint is OK the way it was. As far as I know, it's not even possible to create a unique_ptr<T, D> for a non-move-constructible D. And even if it were, the resolution of LWG3548 doesn't mess with the constraints on this constructor, and this constructor is not specified as being constrained to work only for move-constructible D. So I recommend not touching this line. If you do touch this line, then please make sure you have added a regression test that proves why touching this line is observable. (Obviously if the change is not observable, then we shouldn't make the change.) | |
| 667–668 | Pre-existing: _AllocT > should be _AllocT>. Feel free to fix it. (I blame clang-format, of course :)) | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
| 89 | Line 89 isn't doing anything useful; remove it. Except, actually, let's make this test actually verify that the move ctor is called even for copyable deleter types. struct MovingDeleter {
explicit MovingDeleter(int *moves) : moves_(moves) {}
MovingDeleter(const MovingDeleter&); // never called
MovingDeleter(MovingDeleter&& rhs) : moves_(rhs.moves_) { *moves_ += 1; }
MovingDeleter& operator=(const MovingDeleter&); // never called
void operator()(int *p) const {}
int *moves_;
};
~~~
int moves = 0;
int i = 42;
std::unique_ptr<int, MovingDeleter> u(&i, MovingDeleter(&moves));
moves = 0;
std::shared_ptr<int> s = std::move(u);
assert(moves == 1);
assert(u == nullptr);
assert(s.get() == &i);(Notice that the deleter's operator() doesn't really have to do anything, which lets us reuse it for the int[] case below.) | |
| 226–260 | Drive-by: It would be nice if this test case also asserted some postconditions, similar to what I showed above. int moves = 0; int *p = new int[8]; std::unique_ptr<int[]> u(p); std::shared_ptr<int> s = std::move(u); assert(u == nullptr); assert(s.get() == p); Also, the existence of this test reminds me that you also need a test proving that your fix also applies correctly to unique_ptr<int[], MovingDeleter>. | |
Comment Actions
- Bolster new and existing tests.
- Slight style fixes.
- Mark 3548 as complete in status file.
- Remove misguided addition to constraints on template parameter.
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 283 | I'm not sure this test is helpful, though it does roughly demonstrate s(std::move(u)); is +/= equivilant to s(u.release(), std::move(u.get_deleter())); imo. Happy to remove if reviewers find it unhelpful. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 225–226 | Looks like buildkite is complaining that shared_ptr<int[]> wasn't supported pre-C++14. So these blocks will need to be guarded under #if TEST_STD_VER > 11. | |
| 226–260 | ||
| 281 | Here, we should either have moves == 2 (if everything works the way I'd expect) or simply moves >= 2 (if the behavior is underspecified and we're actually allowed to move as many times as we'd like). I don't immediately see any reason why we should expect line 251 to cause exactly 3 moves. | |
| 292 | What's going on with this line? This test file is supposed to be testing the shared_ptr(unique_ptr&&) ctor, but on this line it seems like you're testing shared_ptr(int*, Deleter) instead. | |
| 295 | Throughout, you seem to be flipping the operands of == pretty much at random (sometimes "Yoda syntax", sometimes normal). Please use normal order throughout, for readability. We expect s.get() to be equal to p, we expect u to be equal to nullptr, etc. This doesn't matter to the computer, but it matters to the human reader. | |
Comment Actions
- Use more readable == operand order in assertions
- Remove irrelevant test case
- Guard std::shared_ptr<T[]> specialization with check for standard version greater than or equal to C++14
Comment Actions
Thanks again for the thoughtful review!
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 281 | RE moves == 2 vs moves >= 2, I assumed the former would be the case. Based off of @philnik 's response offline, it seems like the latter assertion is sufficient:
Nikolas, please correct me if you think I misrepresented your Discord message. | |
| 292 | I intended to demonstrate that the new behavior is equivalent to shared_ptr(r.release(), std::move(r.get_deleter())) as described by the issue report, but you're right, this test is irrelevant here. | |
Comment Actions
LGTM % nit. Looks like you have to guard your test from C++03.
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 276 | Please use TEST_STD_VER > 11 instead. | |
| 281 | Just to clarify: Even if this is a libc++ bug, it shouldn't be fixed in this PR. We should just open a new issue and make a Fix for that. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 235 | Although I'm not convinced we need anything special for C++03 here, actually. Clang does support rvalue references in its non-pedantic C++03 mode, which is all we support; so I'd be surprised if anything behaved differently in C++03 mode. This seems to be an at-least-moderately-deep rabbit hole; see e.g. https://github.com/llvm/llvm-project/blob/main/libcxx/include/__memory/shared_ptr.h#L477-L481 (additional copy instead of move) and https://github.com/llvm/llvm-project/blob/main/libcxx/include/__memory/shared_ptr.h#L237 (additional move instead of nothing). @ashermancinelli, would you like to explore this rabbit hole? If not, I'll do it. Re styling TEST_STD_VER checks: git grep is your friend. git grep -h 'TEST_STD_VER >' ../libcxx | sort | uniq -c | sort -n | |
| 250–258 | The point of using a MovingDeleter is that you don't have to mess with raw new and delete. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 235 | I'd love to take a day or so to look into this rabbit hole, though you would likely find it first, and I won't complain if you do :) | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp | ||
|---|---|---|
| 235 | It seems like the copy constructor isn't needed in C++03 as you observed (after going through compressed_pair and friends), and the link step succeeds after I remove the member function declarations. Buildkite will determine if I counted my eggs too early :) | |
Comment Actions
We discussed making the test look something like:
struct MovingDeleter {
int value;
static int delete_called;
explicit MovingDeleter(int v) : value(v) {}
MovingDeleter(MovingDeleter&& rhs) : value(rhs.value) { rhs.value = -1; }
void operator()(int* p) const {
assert(p);
assert(value == 42);
++delete_called;
}
MovingDeleter& operator=(MovingDeleter&&) = delete;
MovingDeleter(MovingDeleter const&) = delete;
MovingDeleter& operator=(MovingDeleter const&) = delete;
int *moves_;
};
int MovingDeleter::delete_called = 0;
void test() {
{
std::unique_ptr<int, MovingDeleter> u(new int, MovingDeleter(42));
std::shared_ptr<int> s(std::move(u));
}
assert(MovingDeleter::delete_called == 1);
}my only other concern was that we didn't throw between moving the deleter and releasing the unique_ptr. And we shouldn't. The allocator can't show on move, the deleter cannot throw on move, and __enable_weak_this is noexcept, So this implementation should be safe and correct.
Comment Actions
LGTM too, thanks everyone for reviewing.
@ashermancinelli Do you need help committing this? If so, please provide Author Name <email@domain> for attribution, and I'll be happy to commit this on your behalf. Thanks for fixing this!
Comment Actions
Thanks everyone for the helpful review. I've also tested with -O[0-3] as suggested by @EricWF. I will land this by the end of the day. Cheers!
Should the requirement be added here, or should we reuse __shared_ptr_deleter_ctor_reqs? libstdc++ has a similar alias template used here that sfinaes away without move_constructible. That might be more readable, even though the alias template would only be used here iiuc. Also, would it be nicer to use _And?