[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

@philnik Can you also update `RangesAlgorithms.csv`?



================
Comment at: libcxx/include/__algorithm/ranges_min.h:61
+
+    _LIBCPP_ASSERT(__first != __last, "range has to have at least one element");
+
----------------
Question: the Standard has the requirement that the range is not empty for both this overload and the `initializer_list` overload, but it is only checked here. AFAICT, `__min_element::__fn::__go` doesn't do this check. Is there a reason to only apply this check here?


================
Comment at: libcxx/include/__algorithm/ranges_min.h:66
+      if (std::invoke(__comp, std::invoke(__proj, *__first), std::invoke(__proj, __result)))
+        __result = *__first;
+    }
----------------
Some thoughts: this could lead to a lot of copying in the worst case. `min_element` doesn't have this problem because it stores the iterator instead; however, it's fine for `min_element` operating on `forward_iterator`s but cannot be used here since we're iterating an `input_range`. It probably doesn't matter in most scenarios, but in the worst case, I could imagine that calling `min` on `std::vector<ExpensiveToCopy>` could result in copying every single element if the vector happens to be in descending order.

I wonder if it would be worthwhile to have an `if constexpr` to only copy iterators if the range is a forward range, and fall back to this implementation otherwise?


================
Comment at: libcxx/include/__ranges/take_view.h:121
       auto __n = ranges::size(__base_);
-      // TODO: use ranges::min here.
-      return std::min(__n, static_cast<decltype(__n)>(__count_));
+      return ranges::min(__n, static_cast<decltype(__n)>(__count_));
     }
----------------
Thanks for going back to fix this! Sorry about the annoying comment -- do you know if this change affects the visible behavior of `take_view` (e.g., some code compiles now that wouldn't compile previously, or vice versa)? If yes, could you please add a test(s) for this?


================
Comment at: libcxx/include/algorithm:54
+    indirect_strict_weak_order<projected<iterator_t<R>, Proj>> Comp = ranges::less>
+  constexpr borrowed_iterator_t<R> ranges::min(R&& r, Comp comp = {}, Proj proj = {}); // since C++20
 }
----------------
Does the overload taking an `initializer_list` also need to be added?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:21
+//    indirect_strict_weak_order<projected<iterator_t<R>, Proj>> Comp = ranges::less>
+//  constexpr borrowed_iterator_t<R> ranges::min(R&& r, Comp comp = {}, Proj proj = {});
+
----------------
The `initializer_list` overload also seems to be missing from here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:26
+#include <cassert>
+#include <random>
+#include <ranges>
----------------
Nit: `<random>` seems unused?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:43
+  int i;
+  bool operator<(const NotTotallyOrdered& o) const && { return i < o.i; }
+};
----------------
Should this be `TotallyOrderedRval`? Also, wouldn't this class need at least an equality operator to be totally ordered?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:69
+  decltype(auto) ret = std::ranges::min(a[0], a[1], {}, &S::i);
+  ASSERT_SAME_TYPE(decltype(ret), const S&);
+  assert(&ret == &a[1]);
----------------
Nit: it seems like this test uses both `static_assert` and `ASSERT_SAME_TYPE` for comparing types, can it be consistent?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:122
+    assert(ret == 5);
+  }
+  { // test comparator
----------------
Nit: consider adding newlines between braced scopes. I just think it makes it easier to separate different test cases visually.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:160
+
+int main(int, char**) {
+  test();
----------------
It seems like more SFINAE tests are needed -- I don't think constraints such as `input_range`, `copyable`, `indirectly_copyable_storable`, etc. are currently being checked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589