[libcxx-commits] [PATCH] D149615: [libc++][PSTL] Implement std::transform

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__algorithm/pstl_transform.h:35-37
+    _ForwardIterator1 __first,
+    _ForwardIterator1 __last,
+    _ForwardIterator2 __result,
----------------
Per your suggestion, we could name those `_ForwardIterator` and `_ForwardOutIterator`. And for the binary version, `_ForwardIterator1`, `_ForwardIterator2`, and `_ForwardOutIterator`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/pstl.transform.binary.pass.cpp:49
+      std::vector<int> out(std::size(a));
+      auto ret = std::transform(
+          policy,
----------------
We need to test the result type of this algorithm (and all the other PSTL algorithms we have so far, where I think we've neglected that).


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/pstl.transform.binary.pass.cpp:30
+
+template <class Iter1, class Iter2, class Iter3>
+struct Test {
----------------
philnik wrote:
> ldionne wrote:
> > I think we're missing tests for constraints here and in the other test.
> What kind of constraints do you mean?
I meant the execution policy constraint that you added a test for, thanks! I don't know why I left two comments that were so similar, maybe I wanted to point out that it was missing from both the tests.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/pstl.transform.unary.pass.cpp:28
+#include "test_iterators.h"
+
+template <class Iter1, class Iter2>
----------------
Here, let's add a comment explaining "we can't test the constraint on the execution policy, because that would conflict with the binary transform algorithm that doesn't take an execution policy, which is not constrained at all".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149615