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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks a lot for working on this!



================
Comment at: libcxx/include/__algorithm/ranges_transform.h:38
+
+template <class _I1, class _I2, class _O1>
+using binary_transform_result = in_in_out_result<_I1, _I2, _O1>;
----------------
For consistency I think using `_Op` like in the unary type would be better.


================
Comment at: libcxx/include/__algorithm/ranges_transform.h:44
+
+  template <class _InputIterator, class _Sentinel,
+            class _OutputIterator,
----------------
Is there a reason why the naming of the types and variables differs from the other ranges algorithms?
For example `ranges_find.h` uses 
```
  template <input_iterator _Ip, sentinel_for<_Ip> _Sp, class _Tp, class _Proj = identity>
    requires indirect_binary_predicate<ranges::equal_to, projected<_Ip, _Proj>, const _Tp*>
  _LIBCPP_HIDE_FROM_ABI constexpr
  _Ip operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
    auto __pred = [&](auto&& __e) { return std::forward<decltype(__e)>(__e) == __value; };
    return ranges::__find_if_impl(std::move(__first), std::move(__last), __pred, __proj);
  }

```
I think it improves our codebase when we keep these names consistent.
I'm also not to fond of `_InputIterator` and `_OutputIterator`, they are used in the non-range algorithms and there they use the LegacyIterators. So I like that the range based algorithms use a different naming scheme,


================
Comment at: libcxx/include/algorithm:99
      min(R&& r, Comp comp = {}, Proj proj = {});                                    // since C++20
+
+  template<input_iterator I, sentinel_for<I> S, weakly_incrementable O,
----------------
`unary_transform_result` and `binary_transform_result` are also part of the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:111
+      }
+      {
+        int a[] = {1, 2, 3, 4, 5};
----------------
I find this test with the extra whitespace more readable, could you adjust the other test cases in this file accordingly?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:258
+            range1, range2, Out(c), [](int i, int j) { return i + j; });
+
+        assert(base(ret.in1) == a + 1);
----------------



================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:296
+    }
+  }
+}
----------------
I would like a test where an unrelated type is written, for example using a unary lambda like
`[](int i) { return '0' + i }` which returns a char instead of an `int`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:301
+constexpr void test_iterator_in1() {
+  test_iterators<cpp20_input_iterator<int*>, In2, Out, sentinel_wrapper<cpp20_input_iterator<int*>>, Sent2>();
+  test_iterators<forward_iterator<int*>, In2, Out, forward_iterator<int*>, Sent2>();
----------------
I think it would be good to also test `cpp17_input_iterator`, same for `test_iterators_in1_in2`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:320
+constexpr bool test() {
+  test_iterators_in1_in2<cpp17_output_iterator<int*>>();
+  test_iterators_in1_in2<forward_iterator<int*>>();
----------------
We should add a test using the `cpp20_output_iterator` as proposed in D122072. We want to make sure a non-copyable iterator works.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:330
+      std::array<int, 5> b;
+      std::same_as<std::ranges::in_out_result<std::ranges::dangling, int*>> auto ret =
+        std::ranges::transform(std::array{1, 2, 3, 5, 4}, b.data(), [](int i) { return i * i; });
----------------
Should these test also use `decltype(auto)`?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:416
+  static_assert(test());
+}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122173