[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:225
 
     {
+      auto *p = new int[8];
----------------
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`.


================
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));
+      auto *p = new int[8];
+      std::unique_ptr<int[]> u(p);
----------------



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


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


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:266
+        assert(nullptr == u);
+        assert(&i == s.get());
+      }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119159