[libcxx-commits] [PATCH] D143161: [libc++][PSTL] Implement std::{any, all, none}_of

[Louis Dionne via Phabricator via libcxx-commits]

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

This is nice! I would suggest refactoring the serial backend so that we can enable this with `-fexperimental-library` sooner rather than later, maybe after stamping out a couple more algorithms just to confirm there are no unknowns.



================
Comment at: libcxx/include/__algorithm/pstl_any_all_none_of.h:29
+          enable_if_t<is_execution_policy_v<remove_cvref_t<_ExecutionPolicy>>, int> = 0>
+_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI bool
+any_of(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) {
----------------
Can you please make sure you have tests for all nodiscard extensions you're adding?


================
Comment at: libcxx/include/__algorithm/pstl_any_all_none_of.h:35
+      return __pstl::__internal::__parallel_or(
+          {},
+          __policy,
----------------
I suspect we might end up removing those, but until then, we should keep the code self-descriptive.


================
Comment at: libcxx/include/__pstl/internal/parallel_impl.h:69
 //! Return true if brick f[i,j) returns true for some subrange [i,j) of [first,last)
-template <class _BackendTag, class _ExecutionPolicy, class _Index, class _Brick>
-bool
-__parallel_or(_BackendTag __tag, _ExecutionPolicy&& __exec, _Index __first, _Index __last, _Brick __f)
-{
+template <class _BackendTag = __par_backend_tag, class _ExecutionPolicy, class _Index, class _Brick>
+bool __parallel_or(_BackendTag __tag, _ExecutionPolicy&& __exec, _Index __first, _Index __last, _Brick __f) {
----------------
Let's be explicit about the type when we pass it. We can remove it later if not needed.


================
Comment at: libcxx/include/__type_traits/is_execution_policy.h:38-39
+
+template <class _ExecutionPolicy>
+auto&& __remove_parallel_policy(_ExecutionPolicy&&);
+
----------------
Let's return `auto const&` instead, this is kinda confusing for no benefit.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.all_of/pstl.all_of.pass.cpp:35
+  void operator()(Policy&& policy) {
+    int a[] = {1, 2, 3, 4, 5, 6, 7, 8};
+    // simple test
----------------
Let's test on a few sizes of sequences: 1-element, 2-elements, and more than e.g. 100 elements. Not to test the backend itself (which will require its own tests per our discussion), but to test the interaction between the "front-end" of the API and the backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143161