[libcxx-commits] [PATCH] D122982: [libc++] Implement ranges::copy{, _n, _if, _backward}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/copy.h:44
+  if (__libcpp_is_constant_evaluated()
+#ifndef _LIBCPP_COMPILER_GCC
+      && !is_trivially_copyable<_ValueT>::value
----------------
var-const wrote:
> Can you please add a comment to explain the GCC workaround?
Sorry about nitpicking, but reading the comment, I think it would be better phrased as a TODO (something like `Remove this workaround once GCC supports using __builtin_memmove in constexpr contexts`).


================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:43
+  __go(_InIter __first, _Sent __last, _OutIter __result, _Pred& __pred, _Proj& __proj) {
+    for (; __first != __last; ++__first) {
+      if (std::invoke(__pred, std::invoke(__proj, *__first))) {
----------------
philnik wrote:
> var-const wrote:
> > Question: why doesn't this algorithm delegate to the non-ranges version (is it to avoid having to deal with the projection)?
> Why would it? It's a really simple algorithm and has no pitfalls AFAIK.
I don't disagree -- I'm just wondering because it's inconsistent with what `ranges::copy` does, which is to delegate.


================
Comment at: libcxx/include/algorithm:60
 
   template<input_iterator I, sentinel_for<I> S, class T, class Proj = identity>
     requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
----------------
Hmm, looks accidentally deleted?


================
Comment at: libcxx/include/algorithm:113
 
 template <class InputIterator, class Predicate>
     constexpr bool     // constexpr in C++20
----------------
Looks accidentally deleted?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:50
+template <class In, class Out, class Sent = In>
+constexpr void test_iterators() {
+  { // simple test
----------------
philnik wrote:
> var-const wrote:
> > I think the current tests always use trivially copyable values -- can we add at least one test case where values are not trivially copyable?
> `CopyOnce` and `OnlyBackwardsCopyable` aren't trivial.
Yes, but those tests are focused on checking something else. I'd prefer to have a dedicated test for this, even if it's somewhat redundant with the other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122982