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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:70
+constexpr void test(std::array<int, N> a) {
+  (test_iterators(Iters(a.begin()), Iters(a.end())), ...);
+  test_range(a, a.begin(), a.end());
----------------
Quuxplusone wrote:
> Moot style point: Stylistically I prefer `(x , ...)` over `(x, ...)` for the same reason I prefer `(x + ...)` over `(x+ ...)`.
> However, this strikes me as //vastly// too "clever". I think the ultimate reason for this is line 131, which should just be 4 lines. My same comments from the `mismatch` review apply here: let's keep these tests as simple as possible, so we can tell what's going wrong when one fails without a lot of metaprogramming-debugging; and also so that we can be sure our tests are actually testing what we intend, and not being nerfed by some accidental bug in the metaprogramming.
The reason was that I didn't want to run `test_range` multiple times, and having a single fold expression doesn't strike me as very much meta-programming.
I prefer `(x, ...)` for the same reason I write `fun(a, b)` and not `fun(a , b)`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:94-100
+constexpr void test_comparator() {
+    int a[] = {7, 6, 9, 3, 5, 1, 2, 4};
+
+  std::ranges::subrange sr {a, sentinel_wrapper(a + 8)};
+  int* ret = std::ranges::min_element(sr, std::ranges::greater{});
+  assert(ret == a + 2);
+}
----------------
Quuxplusone wrote:
> See, this is a great test, because it's simple enough that I think I see what it's doing. ;)
> Nits: indentation on line 95; prefer `()` over `{}` on line 97. But FWIW, I'd just write
> ```
> {
>   int a[] = {2, 3, 1, 3, 2};
>   int *ret = std::ranges::min_element(a, std::greater());
>   assert(ret == a + 1);
> }
> {
>   const int a[] = {2, 3, 1, 3, 2};
>   const int *ret = std::ranges::min_element(a, std::less());
>   assert(ret == a + 2);
> }
> ```
> or something like that. I don't think we need `sentinel_wrapper` or `subrange` (or `std::ranges::greater`) if the goal is to test the //comparator// codepath specifically.
Why do you want to test `std::less`? It's tested literally every where else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117025