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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but I'd like a bit more test coverage.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max_element.pass.cpp:28
+#include "test_macros.h"
+#include "test_iterators.h"
+
----------------
Would it be possible to use special comparison and projection to validate the complexity?
`Complexity: Exactly max(last - first - 1,0) comparisons and twice as many projections.`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max_element.pass.cpp:83
+}
+
+constexpr void test_borrowed_range_and_sentinel() {
----------------
I would like a test where the maximum value occurs twice to validate whether it returns the first, per `Returns: The first iterator i in the range…`.



================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.max_element.pass.cpp:89
+  int* ret = std::ranges::max_element(sr);
+  assert(ret == a);
+}
----------------
For reviewing it would be nice to also validate the value `assert(*ret == x);` in all these tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117523