[libcxx-commits] [PATCH] D126283: [libc++] Implement ranges::replace{, _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.

The implementation looks great. Just need to add a few test cases and update the synopsis. Thanks!



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:52
 Write,remove_copy_if,Not assigned,n/a,Not started
-Write,replace,Not assigned,n/a,Not started
-Write,replace_if,Not assigned,n/a,Not started
+Write,replace,Nikolas Klauser,` <https://llvm.org/>`_,✅
+Write,replace_if,Nikolas Klauser,` <https://llvm.org/>`_,✅
----------------
Please don't forget to update this with an actual link.


================
Comment at: libcxx/include/algorithm:1003
 #include <__algorithm/ranges_mismatch.h>
+#include <__algorithm/ranges_replace.h>
+#include <__algorithm/ranges_replace_if.h>
----------------
You probably need to update the synopsis as well.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:14
+
+#include <algorithm>
+#include <array>
----------------
Can you also add the synopsis so that the algorithm signature is easy to look up when reading the test file?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:20
+#include "test_iterators.h"
+
+template <class Iter, class Sent, int N>
----------------
Can you also test the `requires` clause and parameter constraints to check that SFINAE works correctly?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:22
+template <class Iter, class Sent, int N>
+constexpr void test(std::array<int, N> a_, int oldval, int newval, std::array<int, N> expected) {
+  {
----------------
Nit: we normally use a trailing underscore for private variables. How about renaming it (maybe to something like `original`, `original_input`, etc.)?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:22
+template <class Iter, class Sent, int N>
+constexpr void test(std::array<int, N> a_, int oldval, int newval, std::array<int, N> expected) {
+  {
----------------
var-const wrote:
> Nit: we normally use a trailing underscore for private variables. How about renaming it (maybe to something like `original`, `original_input`, etc.)?
In the signature, `T1` and `T2` can be different types, but the current tests only cover the case when they are the same type. Can you also test the conversion?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:25
+    auto a = a_;
+    std::same_as<Iter> auto ret = std::ranges::replace(Iter(a.data()), Sent(Iter(a.data() + N)),
+                                                          oldval,
----------------
Nit: `decltype(auto)`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:26
+    std::same_as<Iter> auto ret = std::ranges::replace(Iter(a.data()), Sent(Iter(a.data() + N)),
+                                                          oldval,
+                                                          newval);
----------------
Nit: the formatting looks a little weird here, perhaps align `oldval` and `newval` with `Iter` on the line above?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:43
+  // simple test
+  test<Iter, Sent, 4>({1, 2, 3, 4}, 2, 23, {1, 23, 3, 4});
+  // no match
----------------
Optional: consider adding comments to imitate named arguments, something like:
```
test<Iter, Sent, 4>(/*input=*/{1, 2, 3, 4}, /*old=*/2, /*new=*/23, /*expected=*/{1, 23, 3, 4});
```


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:45
+  // no match
+  test<Iter, Sent, 4>({1, 2, 3, 4}, 5, 23, {1, 2, 3, 4});
+  // all match
----------------
Can this argument be deduced? (in all cases except for `0`, I guess)


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:54
+
+constexpr bool test() {
+  test_iterators<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>();
----------------
Can you also add a test for when the range overload returns `dangling`?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace.pass.cpp:81
+
+int main(int, char**) {
+  test();
----------------
A couple more tests that I think should be added:
1. In the standard, the `operator==` (and the predicate for the `_if` version) isn't required to return a `bool`, just something convertible to a boolean. Can you check those cases?
2. There's a complexity requirement on the predicate and the projection. I know it's trivial, but perhaps test that as well to be exhaustive?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.replace/ranges.replace_if.pass.cpp:22
+template <class Iter, class Sent, int N, class Pred>
+constexpr void test(std::array<int, N> a_, Pred pred, int val, std::array<int, N> expected) {
+  {
----------------
I think all the same comments as in `ranges.replace.pass.cpp` apply here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126283