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

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

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: BRevzin.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:65
+
+    ranges::minmax_result<_Ip> __result = {*__first, *__first};
+    if (__first == __last || ++__first == __last)
----------------
Per @BRevzin https://cpplang.slack.com/archives/C4Q3A3XB8/p1646101005063689 , I think this needs to be
```
ranges::minmax_result<_Ip> __result = {*first, __result.min};
```
because it might not be safe to copy from `*first` twice. You can regression-test this with something like
```
std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
auto [min, max] = std::ranges::minmax(std::move_iterator(a), std::move_iterator(a + 1));
assert(a[0] == nullptr);
assert(min != nullptr);
assert(max == min);
```


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:69-72
+    if (__less(*__first, __result.min))
+      __result.min = *__first;
+    else
+      __result.max = *__first;
----------------
I //think// it's OK to read from `*__first` twice in //this// case, because if `__less(*__first, __result.min)` causes side effects (like moving-from), that's a bug in `__less`, not our problem. But I'm not sure.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:103
+inline namespace __cpo {
+inline constexpr auto minmax = __minmax::__fn{};
+} // namespace __cpo
----------------



================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:97
+inline namespace __cpo {
+inline constexpr auto minmax_element = __minmax_element::__fn{};
+} // namespace __cpo
----------------



================
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;
+      }
----------------
philnik wrote:
> 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?
OK, I think you're right.
I checked out the patch and added calls to `abort()` on each branch of this if-tree to check coverage. That found one untested branch, which you can fix by adding this test case:
```
test<It>({2, 2, 1}, 2, 1);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637