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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:71
+  test_iterators(Iters(a.begin()), Iters(a.end()));
+  test_range(a, a.begin(), a.end());
+}
----------------
I would like to see us test some ranges that are not `std::array`. Given the implementation, I guess it's not likely that anything could go wrong; but still, you could get vastly more mileage out of this test by making it something like
```
template<class It>
constexpr void test_one_case(std::initializer_list<int> a, int expected_idx) {
  int *first = a.begin();
  int *last = a.end();
  {
    std::same_as<It> auto it = std::ranges::min_element(It(first), It(last));
    assert(base(it) == first + expected_idx);
  }
  {
    using Sent = std::sentinel_wrapper<It>;
    std::same_as<It> auto it = std::ranges::min_element(It(first), Sent(It(last)));
    assert(base(it) == first + expected_idx);
  }
  {
    auto r = std::ranges::subrange(It(first), It(last));
    std::same_as<It> auto it = std::ranges::min_element(r);
    assert(base(it) == first + expected_idx);
  }
  {
    using Sent = std::sentinel_wrapper<It>;
    auto r = std::ranges::subrange(It(first), Sent(It(last)));
    std::same_as<It> auto it = std::ranges::min_element(r);
    assert(base(it) == first + expected_idx);
  }
}

template<class It>
constexpr void test_cases()
  test_one_case<It>({}, 0);
  test_one_case<It>({1}, 0);
  test_one_case<It>({1, 2, 1}, 0);
  test_one_case<It>({2, 1, 3}, 1);
  test_one_case<It>({3, 2, 1}, 2);
}

~~~
  test_cases<forward_iterator<int*>>();
  test_cases<bidirectional_iterator<int*>>();
  [etc.]
```


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min_element.pass.cpp:74
+
+template <class... Iters>
+constexpr bool test() {
----------------
Quuxplusone wrote:
> 
Throughout: `s/Iters/Iter/` (or even `It`).
Lines 76-81: Is the first template parameter always just the length of the initializer-list? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117025