[libcxx-commits] [PATCH] D117025: [libc++][ranges] Implement ranges::min_element

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_min_element.h:49
+  borrowed_iterator_t<_Rp> operator()(_Rp&& __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    return operator()(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+  }
----------------
This makes copies of `__comp` and `__proj`. At the very least we should `std::move` them; but I think we should also give serious consideration to just taking them by forwarding reference to begin with.

Speaking of how scary it is to mess with overload sets... what's the mandated behavior for a call like `std::ranges::min_element(42)` — does it need to SFINAE away quietly? If so, we should have a test for that.


================
Comment at: libcxx/include/algorithm:26
+  template<forward_iterator I, sentinel_for<I> S, class Proj = identity,
+    indirect_strict_weak_order<projected<I, Proj>> Comp = ranges::less>             // Since C++20
+  constexpr I min_element(I first, S last, Comp comp = {}, Proj proj = {});
----------------
Nit: lowercase `since C++20` to match line 23 (and generally)


================
Comment at: libcxx/include/module.modulemap:284
       module push_heap                { private header "__algorithm/push_heap.h" }
+      module ranges_min_elemnt        { private header "__algorithm/ranges_min_element.h" }
       module remove                   { private header "__algorithm/remove.h" }
----------------
Typo here. I'll update `lint_modulemap.sh.py`. :)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:30
+
+std::mt19937 randomness;
+
----------------
I see zero reason for this test to use randomness. We just need to check that it works when the first element is the min; when the last element is the min; when some middle element is the min; when there's a tie for min; when the range's size is 1; when the range's size is 0; when the range's value type is immobile (i.e. we shouldn't ever be copying/moving elements).
And then check for SFINAE when the thing isn't a range; when the range's value type isn't `<`-comparable; when it //is// `<`-comparable but not `totally_ordered`.
I would also expect to see the word `std::ranges::dangling` used at least once in this test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117025