var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:47
Write,fill_n,Not assigned,n/a,Not started
-Write,transform,Not assigned,n/a,Not started
+Write,transform,Nikolas Klauser,n/a,✅
Write,generate,Not assigned,n/a,Not started
----------------
Can you add a link to this patch here?
================
Comment at: libcxx/include/__algorithm/ranges_transform.h:44
+
+ template <class _InputIterator, class _Sentinel,
+ class _OutputIterator,
----------------
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.
================
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,
----------------
Nit: `s/first1/first/` (same for `last1`).
================
Comment at: libcxx/include/algorithm:110
+ transform(R&& r, O result, F op, Proj proj = {}); // since C++20
+ template<input_iterator I1, sentinel_for<I1> S1, input_iterator I2, sentinel_for<I2> S2,
+ weakly_incrementable O, copy_constructible F, class Proj1 = identity,
----------------
Nit: can you add blank lines at least between the unary and binary versions of `transform`, and perhaps between all of them? As it stands, I found it a little hard to find where one functions ends and another one starts due to how verbose the template arguments are.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:59
+static_assert(!HasTranformR<InputRangeNotSentinelSemiregular>);
+static_assert(!HasTranformR<InputRangeNotSentinelEqualityComparableWith>);
+
----------------
To make the SFINAE test exhaustive, I think it also needs to check the `weakly_incrementable`, `copy_constructible`, `indirectly_writeable`, etc. constraints.
================
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);
----------------
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);
}
```
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:111
+ }
+ {
+ int a[] = {1, 2, 3, 4, 5};
----------------
Mordante wrote:
> I find this test with the extra whitespace more readable, could you adjust the other test cases in this file accordingly?
+1.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:181
+
+ { // second range empty (binary)
+ {
----------------
Perhaps also check the case when both ranges are empty for binary?
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:209-212
+ }
+
+
+ { // first range one element
----------------
Nit: one extra blank line.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:331
+ 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; });
+ assert((b == std::array{1, 4, 9, 25, 16}));
----------------
Nit: I think continued lines should be indented 4 columns.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.transform/ranges.transform.pass.cpp:415
+ test();
+ static_assert(test());
+}
----------------
Please add `return 0`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122173/new/
https://reviews.llvm.org/D122173