[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_backends/cpu_backends/libdispatch.h:101
     _Compare __comp,
     _LeafMerge __leaf_merge) {
   __chunk_partitions __partitions =
----------------
`noexcept`?


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h:140
   // TODO: Improve the case where the smaller range is merged into just a few (or even one) chunks of the larger case
   std::__terminate_on_exception([&] {
     __merge_range_t* __r = __ranges.get();
----------------
We don't need `__terminate_on_exception` anymore here.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/merge.h:40
     _ForwardOutIterator __result,
     _Comp __comp) {
   if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> &&
----------------
`noexcept`?


================
Comment at: libcxx/include/__algorithm/pstl_backends/pstl_expected.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
As discussed, let's go for `std::optional`. It handles triviality properly and it resolves a lot of complexity that we run into if we try to do `expected`.


================
Comment at: libcxx/include/__algorithm/pstl_merge.h:44
   using _Backend = typename __select_backend<_RawPolicy>::type;
-  return std::__pstl_merge<_RawPolicy>(
-      _Backend{},
-      std::move(__first1),
-      std::move(__last1),
-      std::move(__first2),
-      std::move(__last2),
-      std::move(__result),
-      std::move(__comp));
+  return std::__terminate_on_exception([&] {
+           return std::__pstl_merge<_RawPolicy>(
----------------
We need a test that we behave properly if the iterators throw when they are moved here.

Also note that it would be possible to pass by rvalue ref into `__pstl_merge` and drop `__terminate_on_exception` altogether here (and in fact I don't think we'd need it anywhere anymore, which would be great). But I'm neutral on that, what I care about most is to add tests for this.


================
Comment at: libcxx/include/__numeric/pstl_transform_reduce.h:46
   using _Backend = typename __select_backend<_RawPolicy>::type;
-  return std::__pstl_transform_reduce<_RawPolicy>(
-      _Backend{},
-      std::move(__first1),
-      std::move(__last1),
-      std::move(__first2),
-      std::move(__init),
-      std::move(__reduce),
-      std::move(__transform));
+  return std::__terminate_on_exception([&] {
+           return std::__pstl_transform_reduce<_RawPolicy>(
----------------
Same thing for moving iterators applies here.


================
Comment at: libcxx/include/__numeric/pstl_transform_reduce.h:56
+         })
+      .__get_value_or_throw();
 }
----------------
This could be:

```
*std::__terminate_on_exception(...).or_else([]{ std::__throw_bad_alloc(); });
```

We only need to use some pre-C++23 `or_else` version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154238