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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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>;
----------------
Mordante wrote:
> For consistency I think using `_Op` like in the unary type would be better.
I use `_O1` for consistency with `in_in_out_result` here.


================
Comment at: libcxx/include/__algorithm/ranges_transform.h:44
+
+  template <class _InputIterator, class _Sentinel,
+            class _OutputIterator,
----------------
Mordante wrote:
> var-const wrote:
> > Mordante wrote:
> > > 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,
> > FWIW, I'm mildly in favor of using longer names (though I can see the argument for terser names given how verbose the template parameters are in these functions). I think range algorithms (both committed and in flight) aren't currently consistent in this regard.
> I'm not against longer names in general, but when I read `_InputIterator` I expect a `LegacyIterator` in the `<algorithm>` header. I also would like consistent naming, which can be achieved in two ways. Use the old names in new code or change the old code to use new names.There's a thread on Discord so I think it's easier to discuss it there.
This has been discussed on discord.


================
Comment at: libcxx/include/algorithm:104
+    constexpr ranges::unary_transform_result<I, O>
+      transform(I first1, S last1, O result, F op, Proj proj = {});                 // since C++20
+  template<input_range R, weakly_incrementable O, copy_constructible F,
----------------
var-const wrote:
> Nit: `s/first1/first/` (same for `last1`).
This is consistent with the standard synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:78-84
+        int a[] = {1, 2, 3, 4, 5};
+        int b[5];
+        std::same_as<std::ranges::in_out_result<In1, Out>> decltype(auto) ret =
+          std::ranges::transform(In1(a), Sent1(In1(a + 5)), Out(b), [](int i) { return i * 2; });
+        assert((std::to_array(b) == std::array{2, 4, 6, 8, 10}));
+        assert(base(ret.in) == a + 5);
+        assert(base(ret.out) == b + 5);
----------------
var-const wrote:
> Optional: do you think it would be worthwhile to try reducing duplication here? Perhaps something like:
> 
> ```
> template <class In1, class Out>
> struct Result {
>   std::ranges::in_out_result<In1, Out> ret;
>   Out* out = nullptr;
> };
> 
> // ...
> 
> int a[] = {1, 2, 3, 4, 5};
> int b1[5], b2[5];
> 
> std::same_as<std::ranges::in_out_result<In1, Out>> decltype(auto) ret_iter =
>     std::ranges::transform(In1(a), Sent1(In1(a + 5)), Out(b1), [](int i) { return i * 2; });
> auto range = std::ranges::subrange(In1(a), Sent1(In1(a + 5)));
> std::same_as<std::ranges::in_out_result<In1, Out>> decltype(auto) ret =
>     std::ranges::transform(range, Out(b2), [](int i) { return i * 2; });
> 
> for (const auto& result : {Result{ret_iter, b1}, Result{ret_range, b2}}) {
>   assert((std::to_array(result.out) == std::array{2, 4, 6, 8, 10}));
>   assert(base(result.ret.in) == a + 5);
>   assert(base(result.ret.out) == b + 5);
> }
> ```
This looks way more complicated to make it a bit shorter to me.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:209-212
+  }
+
+
+  { // first range one element
----------------
var-const wrote:
> Nit: one extra blank line.
I think it looks nicer without a blank line here.


================
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; });
----------------
Mordante wrote:
> Should these test also use `decltype(auto)`?
The `decltype(auto)` was specifically because `ranges::min` and friends return `const T&`. But I can also add a `decltype` here if you want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122173