[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 inline comments.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:89
+  MoveOnlyDeleter(MoveOnlyDeleter&&) {}
+  MoveOnlyDeleter(MoveOnlyDeleter&) = delete;
+  void operator()(T* p) const { delete p; }
----------------
Quuxplusone wrote:
> 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.)
Thanks for the awesome suggestions!


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:226
       std::unique_ptr<int[]> ptr(new int[8]);
       std::shared_ptr<int[]> p(std::move(ptr));
     }
----------------
Quuxplusone wrote:
> 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>`.
I realized I cannibalized this test when I should have made a new one. I'll fix that shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119159