[libcxx-commits] [PATCH] D119589: [libc++][ranges] Implement ranges::min

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/algorithm:48
+
+  template<forward_iterator I, sentinel_for<I> S, class Proj = identity,
+    indirect_strict_weak_order<projected<I, Proj>> Comp = ranges::less>
----------------
Hmm, looking at the Standard, I see
```
template<class T, class Proj = identity,
         indirect_­strict_­weak_­order<projected<const T*, Proj>> Comp = ranges::less>
  constexpr const T& ranges::min(const T& a, const T& b, Comp comp = {}, Proj proj = {});

// initializer_list overload looks the same

template<input_­range R, class Proj = identity,
         indirect_­strict_­weak_­order<projected<iterator_t<R>, Proj>> Comp = ranges::less>
  requires indirectly_­copyable_­storable<iterator_t<R>, range_value_t<R>*>
  constexpr range_value_t<R>
    ranges::min(R&& r, Comp comp = {}, Proj proj = {});
```

Should this be fixed (here and in the test file)?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:21
+//          indirect_strict_weak_order<projected<const T*, Proj>> Comp = ranges::less>
+//   constexpr T ranges::min(initializer_list<T> r, Comp comp = {}, Proj proj = {});
+//  template<forward_range R, class Proj = identity,
----------------
Nit: for consistency, please either add a newline between these two overloads, or remove the newline above (I'd prefer the former).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.min.pass.cpp:160
+
+int main(int, char**) {
+  test();
----------------
var-const wrote:
> It seems like more SFINAE tests are needed -- I don't think constraints such as `input_range`, `copyable`, `indirectly_copyable_storable`, etc. are currently being checked.
Hmm, I still don't see a check for `input_range` and `indirectly_copyable_storable`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589