[libcxx-commits] [PATCH] D129806: [libc++] Implement ranges::replace_copy{, _if}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

All comments are pretty minor.



================
Comment at: libcxx/include/__algorithm/ranges_replace_copy_if.h:44
+      *__result = *__first;
+    ++__first;
+    ++__result;
----------------
Optional: consider a blank line above this line.


================
Comment at: libcxx/include/__algorithm/ranges_replace_copy_if.h:47
   }
+  return {std::move(__first), std::move(__result)};
+}
----------------
Nit: can you add a blank line above this line?


================
Comment at: libcxx/include/algorithm:737
+             indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T1*>
+    constexpr replace_copy_result<I, O>
+      replace_copy(I first, S last, O result, const T1& old_value, const T2& new_value,
----------------
Please also add the definitions of the `*_result` types to the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy.pass.cpp:39
+template <class Iter, class Sent = sentinel_wrapper<Iter>, class OutIter = int*>
+concept HasRemoveIfIt =
+    requires(Iter first, Sent last, OutIter result) { std::ranges::replace_copy(first, last, result, 0, 0); };
----------------
`s/RemoveIf/ReplaceCopy/`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy.pass.cpp:80
+
+    std::same_as<std::ranges::in_out_result<InIter, OutIter>> decltype(auto) ret =
+        std::ranges::replace_copy(std::move(first), std::move(last), std::move(result), d.old_value, d.new_value);
----------------
Should this be `replace_copy_result`? Also, you might need to update `ranges_result_alias_declarations.compile.pass.cpp`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy.pass.cpp:170
+
+  {   // check complexity requirements
+    { // iterator overload
----------------
Optional: it would be convenient to copy the exact requirement from the standard here.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy_if.pass.cpp:42
+template <class Iter, class Sent = sentinel_wrapper<Iter>, class OutIter = int*>
+concept HasRemoveIfIt = requires(
+    Iter first, Sent last, OutIter result) { std::ranges::replace_copy_if(first, last, result, FalsePredicate{}, 0); };
----------------
Same as the other file, the name needs to be updated.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy_if.pass.cpp:55
+template <class Range, class OutIter = int*>
+concept HasReplaceCopyR =
+    requires(Range range, OutIter result) { std::ranges::replace_copy_if(range, result, FalsePredicate{}, 0); };
----------------
`s/ReplaceCopy/ReplaceCopyIf/`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy_if.pass.cpp:86
+
+    std::same_as<std::ranges::in_out_result<InIter, OutIter>> decltype(auto) ret =
+        std::ranges::replace_copy_if(std::move(first), std::move(last), std::move(result), pred, d.new_value);
----------------
Same nit re. return type.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges_replace_copy_if.pass.cpp:186
+      std::ranges::replace_copy_if(std::begin(a), std::end(a), std::begin(b), FalsePredicate{}, 0, proj);
+      assert(proj_count == 4);
+    }
----------------
I think you also need to check the number of predicate invocations. There's some prior art in `test_support/counting_predicates.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129806