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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

LGTM with just a couple of minor things. I'll leave the final approval to @ldionne since he had comments.



================
Comment at: libcxx/include/__iterator/reverse_iterator.h:336
+#endif
+  return pair<reverse_iterator<_Iter>, reverse_iterator<_Iter>>(std::move(__rbegin), std::move(__rend));
+}
----------------
`make_pair`?


================
Comment at: libcxx/include/algorithm:396
+    requires indirectly_copyable<I, O>
+    constexpr ranges::reverse_copy_result<I, O>
+      ranges::reverse_copy(I first, S last, O result);                                      // since C++20
----------------
Please add the definition of `{reverse,rotate}_copy_result` to the synopsis as well.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp:29
+#include "almost_satisfies_types.h"
+#include "test_iterators.h"
+
----------------
ldionne wrote:
> I would like an explicit test that `is_same<reverse_copy_result<....>, in_out_result<...>>`. In fact, I don't see that type mentioned anywhere in the tests, so we wouldn't even notice if we didn't define it. Please do that here but also for all the other algorithms that have been added and that had accompanying `foo_result` types.
Maybe create a dedicated file for this, similar to `ranges_robust_against_copying_*`? It seems like these checks will be really similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127211