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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

LGTM % nit. Looks like you have to guard your test from C++03.



================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp:246
+
+#if TEST_STD_VER >= 14
+      {
----------------
Please use `TEST_STD_VER > 11` instead.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119159