[libcxx-commits] [PATCH] D120637: [libc++][ranges] Implement ranges::minmax and ranges::minmax_element

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:40
+  minmax_result<_Ip> __go(_Ip __first, _Sp __last, _Comp __comp, _Proj __proj) {
+    auto __project = [&](_Ip& __p) -> decltype(auto) { return std::invoke(__proj, *__p); };
+
----------------
Quuxplusone wrote:
> Given how you use this, I think it could profitably be more like
> ```
> auto __less = [&](_Ip& __it, _Ip& __jt) -> bool {
>   return std::invoke(__comp, std::invoke(__proj, *__it), std::invoke(__proj, *__jt));
> };
> ```
> I'm ambivalent whether the params should be `const _Ip&`. If it's legal, then I'm leaning toward "yes."
I think it wouldn't work if the comparator expects non-const parameters,.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:69-70
+          __result.min = __i;
+        if (!std::invoke(__comp, __project(__first), __project(__result.max)))
+          __result.max = __first;
+      }
----------------
Quuxplusone wrote:
> In my head this gives the wrong answer for `{1,2,4,4}`; `max` in this case should point to the second `4`, not the first `4`. I may have done the mental simulation wrong, though. Please investigate and add a regression test if needed — in fact, I recommend adding a regression test either way!
This is already tested with `test<It>({2, 1, 2}, 1, 2);` or am I missing something?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp:176
+    assert(ret.min.i == 1);
+    assert(ret.max.i == 3);
+  }
----------------
Quuxplusone wrote:
> Please add
> ```
> ret = std::ranges::minmax(b, b + 3, {}, &S::i);
> assert(ret.min.i == 1);
> assert(ret.max.i == 3);
> ```
> so we test the iterator-sentinel version with `S` as well.
There is no iterator-sentinel version for `ranges::minmax`? Did you mean `ranges::minmax_element`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637