[libcxx-commits] [PATCH] D122173: [libc++][ranges] Implement ranges::transform

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think this is good to go after my suggestions. I'll want to see this again just to double check, but there shouldn't be anything else to change. Thanks!



================
Comment at: libcxx/include/__algorithm/ranges_transform.h:35-39
+template <class _Ip, class _Op>
+using unary_transform_result = in_out_result<_Ip, _Op>;
+
+template <class _I1, class _I2, class _O1>
+using binary_transform_result = in_in_out_result<_I1, _I2, _O1>;
----------------
Can you please make sure you have tests that check that we define those two names in `std::ranges` specifically, i.e. not as part of testing `ranges::transform()` itself? This also applies to other reviews like `minmax_element`.


================
Comment at: libcxx/include/__algorithm/ranges_transform.h:49
+  _LIBCPP_HIDE_FROM_ABI static constexpr
+  unary_transform_result<_InIter, _OutIter> __unary(_InIter __first, _Sent __last,
+                                                    _OutIter __result,
----------------
Can you make `__unary` and `__binary` private?


================
Comment at: libcxx/include/algorithm:100-104
+  template <class _Ip, class _Op>
+  using unary_transform_result = in_out_result<_Ip, _Op>;                           // since C++20
+
+  template <class _I1, class _I2, class _O1>
+  using binary_transform_result = in_in_out_result<_I1, _I2, _O1>;                  // since C++20
----------------
Those shouldn't use uglified names.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:214
+
+        std::same_as<std::ranges::in_in_out_result<In1, In2, Out>> decltype(auto) ret = std::ranges::transform(
+            range1, range2, Out(c), [](int i, int j) { return i + j; });
----------------
You could consider checking the return type only once for each relevant call, and then use `auto` elsewhere. That would simplify these tests quite a bit.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:393-397
+  // test_iterator_in1<forward_iterator<int*>, Out>();
+  // test_iterator_in1<bidirectional_iterator<int*>, Out>();
+  // test_iterator_in1<random_access_iterator<int*>, Out>();
+  // test_iterator_in1<contiguous_iterator<int*>, Out>();
+  // test_iterator_in1<int*, Out>();
----------------
This shouldn't be commented out (ditto below).


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:408
+
+  { // check that std::ranges::dangling is returned properly
+    { // unary
----------------
Would it be possible to move these tests into the `test_iterators` function? LMK if compile-times explode in a bad way.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:444
+
+  { // count predicate and projection call counts
+    { // unary
----------------
I don't think you mean "predicate" here, maybe just "function" is what you mean.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:492
+
+  { // check that returning another type works
+    { // unary
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122173