[libcxx-commits] [PATCH] D128618: [libc++] Implement ranges::remove{, _if}

[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/CMakeLists.txt:97
   __algorithm/ranges_max_element.h
   __algorithm/ranges_min.h
   __algorithm/ranges_min_element.h
----------------
Please update the status page, the module map, and the private headers test file.


================
Comment at: libcxx/include/__algorithm/ranges_remove.h:37
+    auto __pred = [&](auto&& __other) { return __value == __other; };
+    return ranges::__remove_if_impl(__first, __last, __pred, __proj);
+  }
----------------
Nit: please move the iterators.


================
Comment at: libcxx/include/__algorithm/ranges_remove_if.h:37
+subrange<_Iter> __remove_if_impl(_Iter __first, _Sent __last, _Pred& __pred, _Proj& __proj) {
+  auto __end = ranges::__find_if_impl(__first, __last, __pred, __proj);
+  if (__end == __last)
----------------
I found the name `__end` a little confusing -- it's not obvious how it is different from `__last`. How about `new_end` or similar?


================
Comment at: libcxx/include/algorithm:1214
 #include <__algorithm/ranges_max_element.h>
 #include <__algorithm/ranges_min.h>
 #include <__algorithm/ranges_min_element.h>
----------------
Please add synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:16
+//   requires indirect_binary_predicate<ranges::equal_to, projected<I, Proj>, const T*>
+//   constexpr subrange<I> ranges::remove(I first, S last, const T& value, Proj proj = {});
+// template<forward_range R, class T, class Proj = identity>
----------------
Nit: wrong synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:47
+template <class Range>
+concept HasRemoveR = requires(Range range) { std::ranges::remove(range, 0); };
+
----------------
`remove_if`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:53
+static_assert(!HasRemoveR<SentinelForNotSemiregular>);
+static_assert(!HasRemoveR<SentinelForNotWeaklyEqualityComparableWith>);
+static_assert(!HasRemoveR<UncheckedRange<int**>>); // not indirect_unary_predicate
----------------
Please also check `permutable`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:65
+constexpr void test(Data<N, M> d) {
+  {
+    auto input = d.input;
----------------
Please add brief comments to explain what is being tested.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:69
+    std::same_as<std::ranges::subrange<Iter>> decltype(auto) ret = std::ranges::remove_if(
+        Iter(input.data()), Sent(Iter(input.data() + input.size())), [&](int i) { return i < d.val; });
+
----------------
Optional: consider using some intermediate variables to make this expression a little easier to parse.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:91
+constexpr void tests() {
+  // simple test
+  test<Iter, Sent, 6, 2>({.input = {1, 2, 3, 4, 5, 6}, .expected = {5, 6}, .val = 5});
----------------
Can you please add a more complicated test case that contains the following?
- two or more matching elements in a row;
- a matching element followed by a non-matching element;
- a matching element followed by several non-matching elements?

Also might be worthwhile to check both the case where the last element in the range matches and the case when it doesn't match.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:92
+  // simple test
+  test<Iter, Sent, 6, 2>({.input = {1, 2, 3, 4, 5, 6}, .expected = {5, 6}, .val = 5});
+  // empty range
----------------
It's not obvious from the name `val` that the predicate is `< val` -- perhaps name it `max`, `cutoff{,_point}` or similar?


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:101
+  test<Iter, Sent, 2, 1>({.input = {1, 2}, .expected = {2}, .val = 2});
+  // all elements matche
+  test<Iter, Sent, 5, 0>({.input = {1, 1, 1, 1, 1}, .expected = {}, .val = 2});
----------------
Nit: `s/matche/match/`.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:101
+  test<Iter, Sent, 2, 1>({.input = {1, 2}, .expected = {2}, .val = 2});
+  // all elements matche
+  test<Iter, Sent, 5, 0>({.input = {1, 1, 1, 1, 1}, .expected = {}, .val = 2});
----------------
var-const wrote:
> Nit: `s/matche/match/`.
How about also `No elements match`? (for a non-trivial case with several elements)


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.remove/ranges.remove_if.pass.cpp:131
+  {// check complexity requirements
+   {int comp_count = 0;
+  auto comp = [&](int i) {
----------------
Nit: weird formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128618