[libcxx-commits] [PATCH] D155330: [libc++][PSTL] Implement std::move

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments applied.



================
Comment at: libcxx/include/__algorithm/pstl_move.h:54
+      [&__policy](_ForwardIterator __g_first, _ForwardIterator __g_last, _ForwardOutIterator __g_result) {
+        return std::__transform(__policy, __g_first, __g_last, __g_result, [](auto&& __v) { return std::move(__v); });
+      },
----------------
Right now, we perform a move-construction + an assignment for every element (https://godbolt.org/z/vTs9ofrrT).

This can be fixed with `-> decltype(auto)` on the lambda (https://godbolt.org/z/WKeMdEc3W)

Let's fix this and add a test. This is an important property of `std::move`!


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
philnik wrote:
> ldionne wrote:
> > You need to add a test that `__move` is `noexcept`, we said we would add such tests in the PSTL / exception safety patch.
> I thought we didn't need the test after all, since `noexcept` isn't necessary on the backend anymore and the death tests caught missing `noexcepts` quite well (and a std tests seems generally preferable to libc++-specific ones).
I think you're right, I just got confused with all the recent discussions we had about exception safety in PSTL.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you add tests with move-only types? This should definitely work since it's `std::move`.

Can you also take a look at the serial `std::move` tests to make sure we test it there? We should, but just to be certain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155330