[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 a comment.
This revision now requires changes to proceed.

The code looks great at this point! Significant comments on the test.



================
Comment at: libcxx/include/__algorithm/ranges_min_element.h:34
+struct __fn {
+
+  template <class _Ip, class _Sp, class _Proj, class _Comp>
----------------
Remove blank line.


================
Comment at: libcxx/include/__algorithm/ranges_min_element.h:43
+    while (++__i != __last)
+      if (bool(_VSTD::invoke(__comp, _VSTD::invoke(__proj, *__i), _VSTD::invoke(__proj, *__first))))
+        __first = __i;
----------------
`bool(`...`)` is unnecessary here, because the `invoke_result_t` of a `predicate` is guaranteed to be //`boolean-testable`//. If someone thinks the cast looks nice for some reason, I won't complain too much; but IMHO you should eliminate the cast as unnecessary.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:32-33
+
+template <class T>
+concept MinElementCallable = requires (T i) { std::ranges::min_element(i); };
+
----------------
Please either name this concept `HasMinElement`, or just use
`static_assert(!std::is_invocable_v<decltype(std::ranges::min_element), int&>);`
etc. without defining a named concept at all. I have a slight preference for the latter.
(Also, surely `T i` should be `T t` or perhaps `R& r`. Notice that you only ever pass lvalues here; that's one reason it would be clearer to use `is_invocable_v` instead of the named concept.)


================
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());
----------------
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.


================
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);
+}
----------------
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.


================
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);
----------------
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);
```


================
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>{}))>);
+
----------------
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);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117025