[libcxx-commits] [PATCH] D127211: [libc++] Implement ranges::{reverse, rotate}_copy

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_reverse_copy.h:51
+  reverse_copy_result<borrowed_iterator_t<_Range>, _OutIter> operator()(_Range&& __range, _OutIter __result) const {
+    auto __ret = ranges::copy(__range | views::reverse, std::move(__result));
+    return {ranges::next(ranges::begin(__range), ranges::end(__range)), std::move(__ret.out)};
----------------
var-const wrote:
> While this allows for a very elegant implementation, the view is way more heavyweight than a simple eager implementation would be. I'd rather avoid the dependency: it adds compilation time overhead, gives users a way to accidentally depend on something they don't include directly, and it might be less efficient (caching logic looks suspicious in that regard).
> 
> Whichever approach we settle upon will likely affect some other algorithms as well, so let's check with @ldionne.
> 
I think I'd be OK with this. We're trying to granularize headers so that this kind of reuse becomes possible. And we do have an optimization for reverse iterators in `std::__copy`, so this should end up being as efficient as a hand-written implementation for contiguous iterators.

I would suggest creating a really simple utility, something like `std::__reverse_range(rng)` that returns a pair of `reverse_iterator`s from a range. This basically does the job of `reverse_view`, but without the need for caching and any other complicated logic. It only needs to have logic for using `std::make_reverse_iterator(ranges::end(rng))` or `std::make_reverse_iterator(ranges::next(ranges::begin(rng), ranges::end(rng)))` depending on whether `rng` is a common view. I suspect this would tackle the concern of pulling in too many dependencies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127211