[libcxx-commits] [PATCH] D142864: [libc++] `<algorithm>`: `ranges::minmax` should dereference iterators only once

[Igor Zhukov via Phabricator via libcxx-commits]

fsb4000 created this revision.
fsb4000 added a project: libc++.
Herald added a project: All.
fsb4000 requested review of this revision.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

In https://reviews.llvm.org/D135248 `libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp` failed because move_iterator became `random_access_iterator` and `ranges::minmax` now chooses this branch.
We can't do `*__result.first` twice because move_iterator will move the value after the first dereference.

This is basically the same bug as I found in MS STL two days ago: https://github.com/microsoft/STL/pull/3366

How can I create the same testcase for  input_iterators branch?

I mean after https://reviews.llvm.org/D135248 will be merged the code:

  c++
  std::shared_ptr<int> a[] = {std::make_shared<int>(42)};
  auto range      = std::ranges::subrange(std::move_iterator(a), std::move_iterator(a + 1));
  auto [min, max] = std::ranges::minmax(range);
  assert(a[0] == nullptr);
  assert(min != nullptr);
  assert(max == min);

will not test the else branch:

  // input_iterators can't be copied, so the implementation for input_iterators has to store
  // the values instead of a pointer to the correct values


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142864

Files:
  libcxx/include/__algorithm/ranges_minmax.h


Index: libcxx/include/__algorithm/ranges_minmax.h
===================================================================
--- libcxx/include/__algorithm/ranges_minmax.h
+++ libcxx/include/__algorithm/ranges_minmax.h
@@ -74,6 +74,15 @@
 
     if constexpr (forward_range<_Range>) {
       auto __result = std::__minmax_element_impl(__first, __last, __comp, __proj);
+      // We can dereference normal pointers many times but some iterators could move their value after
+      // dereferencing so we should dereference them only once. The if constexpr is a pure optimization, we
+      // don't want an additional branch for normal pointers.
+      if constexpr (!is_pointer_v<iterator_t<_Range>>) {
+        if (__result.first == __result.second) {
+          auto __temp = static_cast<_ValueT>(*__result.first);
+          return {__temp, std::move(__temp)};
+        }
+      }
       return {*__result.first, *__result.second};
     } else {
       // input_iterators can't be copied, so the implementation for input_iterators has to store


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142864.493196.patch
Type: text/x-patch
Size: 1039 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230130/ec1bcf71/attachment.bin>