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

Fri Feb 4 09:16:28 PST 2022

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Let's fix that variadic `test` function. It's cheap and easy to fix.

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:74
+
+template <class... Iters>
+constexpr bool test() {
----------------

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:149-152
+  test<forward_iterator<const int*>,
+       bidirectional_iterator<const int*>,
+       random_access_iterator<const int*>,
+       const int*>();
----------------

================
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());
----------------
philnik wrote:
> 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)`.
> I prefer `(x, ...)` for the same reason I write `fun(a, b)` and not `fun(a , b)`.

I would have said the same thing! ;) I write `fun(a, b)` and `{1, 2}` and so on because each of those commas represents the comma //separator//. But when I use the comma //operator//, at least in a fold-expression context where the reader might casually mistake it for the comma separator, I'll call out that unusual usage by formatting it like an operator.

================
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);
+}
----------------
philnik wrote:
> 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.
Well, in general TDD terms, testing two different comparators "proves" that the implementation isn't just "if there's a comparator object then reverse all the comparisons" or something. But I don't mind just testing `std::greater`.
(I'd still prefer `std::greater()` over `std::ranges::greater{}`, purely for style: it's shorter and causes fewer dependencies. But whatever.)
No further action required here.

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:106
+  std::ranges::subrange sr {a, sentinel_wrapper(a + 8)};
+  int* ret = std::ranges::min_element(sr, std::ranges::less{}, [](int i) { return i == 5 ? -100 : i; });
+  assert(ret == a + 4);
----------------
Quuxplusone wrote:
> Nice example of a call where the answer would be different if the predicate weren't there!
> I'd like to see one additional test where the projected type is not the same type as the range's value_type, e.g.
> ```
> int a[] = {2, 1, 3};
> int *ret = std::ranges::min_element(a, std::less<int*>(), [](int& i) { return &i; });
> assert(ret == a);
> ```
Here //please// use `std::less<int*>()` with the specific type `int*`, because I want to verify that the comparator doesn't need to be callable with the original element type — we want it to be callable (and called) only with the projected type.

================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:127
+
+static_assert(std::is_same_v<std::ranges::dangling, decltype(std::ranges::min_element(std::array<int, 0>{}))>);
+
----------------
Quuxplusone wrote:
> Moot: `, 0` is kind of a weird corner case; I'd make this `, 10` or `, 42`.
> But also, we need to test this behavior outside an unevaluated expression. I suggest
> ```
> constexpr void test_dangling()
> {
>   int a[] = {2, 1, 3};
>   int compares = 0;
>   int projections = 0;
>   auto comparator = [&](int a, int b) { compares += 1; return a < b; };
>   auto projection = [&](int a) { projections += 1; return a; };
>   std::same_as<std::ranges::dangling> auto r =
>     std::ranges::min_element(r, comparator, projection);
>   assert(compares == 2);
>   assert(projections == 4);
> }
> ```
You can remove line 128 (the static_assert) now, which will allow you to remove `#include <array>` (and eliminate the weirdness of `std::array<T, 0>`).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117025