[libcxx-commits] [PATCH] D154238: [libc++][PSTL] Move exception handling to the CPU backends

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/pstl_any_all_none_of.h:14
 #include <__algorithm/pstl_frontend_dispatch.h>
 #include <__config>
 #include <__iterator/cpp17_iterator_concepts.h>
----------------
Missing `<optional>` include. Please make a pass throughout.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/any_of.h:21
 #include <__type_traits/is_execution_policy.h>
 #include <__utility/pair.h>
 #include <cstdint>
----------------
`<__utility/move.h>`


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h:33
+_LIBCPP_HIDE_FROM_ABI optional<_Index>
+__parallel_find(_Index __first, _Index __last, _Brick __f, _Compare __comp, bool __b_first) noexcept {
   typedef typename std::iterator_traits<_Index>::difference_type _DifferenceType;
----------------
This seems like a leftover, we decided to only mark the top-level `__find` algo `noexcept`.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h:38
+_LIBCPP_HIDE_FROM_ABI optional<__empty>
+__parallel_for(_RandomAccessIterator __first, _RandomAccessIterator __last, _Fp __f) noexcept {
   __f(__first, __last);
----------------
`noexcept` leftover


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform.h:44
     _ForwardOutIterator __result,
-    _UnaryOperation __op) {
+    _UnaryOperation __op) noexcept {
   if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
----------------
`noexcept` leftover


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform.h:91
     _ForwardOutIterator __result,
-    _BinaryOperation __op) {
+    _BinaryOperation __op) noexcept {
   if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
----------------
`noexcept` leftover


================
Comment at: libcxx/include/__algorithm/pstl_is_partitioned.h:60
+_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI bool
+is_partitioned(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) {
+  auto __res = std::__is_partitioned(__policy, std::move(__first), std::move(__last), std::move(__pred));
----------------
In another patch, can you please make a pass to make sure you're not missing `_LLIBCPP_REQUIRE_CPP17_FOO` from any of them?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Let's name this test `pstl.exception_handling.pass.cpp` since this isn't only specific to `std::fill`. WDYT?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp:13
+
+// check that std::fill(ExecutionPolicy) terminates properly
+
----------------
You can adjust all the comments


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp:13
+
+// check that std::fill(ExecutionPolicy) terminates properly
+
----------------
ldionne wrote:
> You can adjust all the comments
Also you should mention that you're testing `std::fill_n` here (this applies to all these tests).


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp:40
+      }
+      std::terminate();
+    });
----------------
everywhere


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp:43
+
+    EXPECT_STD_TERMINATE([&] { (void)std::fill_n(policy, std::begin(a), std::size(a), ThrowOnCopy{}); });
+    EXPECT_STD_TERMINATE([&] {
----------------
Please add a comment like `// test std::fill_n` to separate each algorithm you're testing. This is kinda dumb here cause there's 2, but when there's more it's useful to have a visual marker


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/pstl.for_each.terminate.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think you need to `UNSUPPORT` these tests when `-fno-exceptions`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154238