[libcxx-commits] [PATCH] D119159: [libc++] Make shared_ptr move unique_ptr's deleter

[Asher Mancinelli via Phabricator via libcxx-commits]

ashermancinelli marked 5 inline comments as done.
ashermancinelli added a comment.

Thanks again for the thoughtful review!



================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:252
+        std::shared_ptr<int[]> s(std::move(u));
+        assert(4 == moves);
+        assert(nullptr == u);
----------------
Quuxplusone wrote:
> 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.
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:

> You can ignore [the number of moves differing between `s(u.release(), std::move(u.get_deleter()));` and `s(std::move(u))`]. If the implementation isn't really weird there should be exactly one more move call with a moved-in object than with a copied-in (unless I'm missing something). You can make a test for that if you want under libcxx, but I don't think that's necessary. Maybe not even a good idea.

Nikolas, please correct me if you think I misrepresented your Discord message.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:263
+        assert(1 == moves);
+        std::shared_ptr<int> s(u.release(), std::move(u.get_deleter()));
+        assert(5 == moves);
----------------
Quuxplusone wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119159/new/

https://reviews.llvm.org/D119159