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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_min.h:50-51
+  _Tp operator()(initializer_list<_Tp> __r, _Comp __comp = {}, _Proj __proj = {}) const {
+    _LIBCPP_ASSERT(ranges::begin(__r) != ranges::end(__r), "initializer_list must contain at least one element");
+    return *ranges::__min_element::__fn::__go(ranges::begin(__r), ranges::end(__r), __comp, __proj);
+  }
----------------
(1) I was going to comment "Is it safe to call `ranges::begin` twice on an arbitrary range?" (I think not). But then noticed that this is just an initializer_list. Please `s/__r/__il/g` as a matter of naming.
Then, I also strongly recommend replacing `ranges::begin/end` with `__il.begin/end()`; but if this is because we're following the Standard's wording exactly, then it's OK to disregard me here.

(2) Depending on how https://reviews.llvm.org/D121248#inline-1160030 is resolved, this may need to become `ranges::__min_element_impl(...)`.  @ldionne, PTAL at my argument over there; WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119589