[libcxx-commits] [PATCH] D117523: [libc++][ranges] Implement ranges::max_element

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM % comments, and the only significant comment affects the already-committed test for `ranges::min_element`. I think this is good to go, to get min and max back into parity; but then after that, they should both get some improved tests.

Also, before landing, please run a `diff <(sed s/min/foo/g ranges_min_element.h) <(sed s/max/foo/g ranges_max_element.h)` and make sure there are no extraneous differences between the two source files. If I understand correctly, there should only be one diff: the order of the arguments to `__comp`!



================
Comment at: libcxx/include/__algorithm/ranges_max_element.h:34
+  template <class _Ip, class _Sp, class _Proj, class _Comp>
+  _LIBCPP_HIDE_FROM_ABI constexpr static
+  _Ip __go(_Ip __first, _Sp __last, _Comp& __comp, _Proj& __proj) {
----------------
Nit: `static constexpr`
https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max_element.pass.cpp:57-66
+template <class Range, class Iter>
+constexpr void test_range(Range&& rng, Iter begin, Iter end) {
+  std::same_as<Iter> auto it = std::ranges::max_element(std::forward<Range>(rng));
+  if (begin != end) {
+    for (Iter j = begin; j != end; ++j)
+      assert(!(*j > *it));
+  } else {
----------------
`test_range` is called only once — both here //and// in `ranges.min_element.pass.cpp`, although I feel sure I commented multiple times about this kind of thing. Replace its call-site there with
```
  int a2[] = {7, 6, 5, 3, 4, 2, 1, 8};
  assert(std::ranges::min_element(a) == a + 6);
```
and here with
```
  int a2[] = {7, 6, 5, 3, 4, 2, 1, 8};
  assert(std::ranges::max_element(a) == a + 7);
```
After that, think about whether there's any test coverage missing. (Hint: probably.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117523