[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.h:65
+
+    ranges::minmax_result<_Ip> __result = {*__first, *__first};
+    if (__first == __last || ++__first == __last)
----------------
ldionne wrote:
> Quuxplusone wrote:
> > 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);
> > ```
> I don't *think* `__result` is considered initialized until the semicolon, though, which means that we'd be accessing an object before it has been initialized, which is UB. Why can't we do this instead:
> 
> ```
> auto __tmp = *__first;
> ranges::minmax_result<_Ip> __result = {__tmp, std::move(__tmp)};
> ```
> 
> I asked a question in the thread, waiting for an answer.
I think http://eel.is/c++draft/dcl.init.aggr#7 is the line in question here, and that sounds to me like the first element has to be fully initialized before the second one.


================
Comment at: libcxx/include/__algorithm/ranges_minmax_element.h:60
+
+  while (++__first != __last) {
+    _Iter __i = __first;
----------------
ldionne wrote:
> Per our discussion: it would be interesting to check which of the following options is best in terms of codegen:
> 
> 1. Use `[[unlikely]]` on the `if (++__first == __last)` branch to tell the compiler that we rarely hit the end of the range, or
> 2. If we have random-access iterators, handle the case of an odd-sized range explicitly above so we can get rid of the `if (++__first == __last)` check in the loop altogether and always process two elements at a time, or
> 3. Leave the code as-is
https://quick-bench.com/q/ezc6hrE4rQ4Smt-XXF72ENAx3iI
It looks like using `[[unlikely]]` makes no difference and checking the size before the loop is slightly slower.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120637