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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.

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.)



================
Comment at: libcxx/include/__memory/shared_ptr.h:656
+         is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>::value &&
+         is_move_constructible<_Dp>::value
     > >
----------------
philnik wrote:
> ashermancinelli wrote:
> > 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`?
> I think it's OK the way it is. Don't use `_And` if there is no good reason.
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.)


================
Comment at: libcxx/include/__memory/shared_ptr.h:669
             typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
             typedef __shared_ptr_pointer<typename unique_ptr<_Yp, _Dp>::pointer, _Dp, _AllocT > _CntrlBlk;
+            __cntrl_ = new _CntrlBlk(__r.get(), std::move(__r.get_deleter()), _AllocT());
----------------
Pre-existing: `_AllocT >` should be `_AllocT>`. Feel free to fix it. (I blame clang-format, of course :))


================
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; }
----------------
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.)


================
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));
     }
----------------
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>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119159