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

[Igor Zhukov via Phabricator via libcxx-commits]

fsb4000 updated this revision to Diff 493778.
fsb4000 added a comment.

@philnik

> You have to add a regression test for this.

We have a test for this.

  {
      // check that the range_access iterator isn't moved from multiple times
      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);
    }

after std::move_iterator will become a range access iterator. But I've added a test for input_move_iterator not to lose testing the else branch in the algorithm range::minmax.

> IMO we should special-case the one element case here instead.

Maybe I misunderstood but we not always can call `ranges::size` to find out that range size is one but we always can check equality iterators...


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

https://reviews.llvm.org/D142864

Files:
  libcxx/include/__algorithm/ranges_minmax.h
  libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp


Index: libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp
===================================================================
--- libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp
+++ libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp
@@ -334,12 +334,51 @@
   return true;
 }
 
+class input_move_iterator
+{
+public:
+  using iterator_category = std::input_iterator_tag;
+  using iterator_concept = std::input_iterator_tag;
+  using difference_type = std::ptrdiff_t;
+  using value_type = std::shared_ptr<int>;
+  using pointer = std::shared_ptr<int>*;
+  using reference = std::shared_ptr<int>&&;
+
+  input_move_iterator() = default;
+  input_move_iterator(std::shared_ptr<int>* ptr)
+  : m_ptr(ptr)
+  {}
+
+  reference operator*() const { return std::ranges::iter_move(m_ptr); }
+  pointer operator->() { return m_ptr; }
+
+  input_move_iterator& operator++() {
+    m_ptr++;
+    return *this;
+  }
+  input_move_iterator operator++(int) {
+    input_move_iterator tmp = *this;
+    ++(*this);
+    return tmp;
+  }
+
+  friend bool operator== (const input_move_iterator& a, const input_move_iterator& b) {
+    return a.m_ptr == b.m_ptr;
+  };
+  friend bool operator!= (const input_move_iterator& a, const input_move_iterator& b) {
+    return a.m_ptr != b.m_ptr;
+  };
+
+  private:
+    std::shared_ptr<int>* m_ptr;
+};
+
 int main(int, char**) {
   test();
   static_assert(test());
 
   {
-    // check that the iterator isn't moved from multiple times
+    // check that the range_access iterator isn't moved from multiple times
     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);
@@ -347,6 +386,15 @@
     assert(min != nullptr);
     assert(max == min);
   }
+  {
+    // check that the input iterator isn't moved from multiple times
+    std::shared_ptr<int> a[] = { std::make_shared<int>(42) };
+    auto range = std::ranges::subrange(input_move_iterator(a), input_move_iterator(a + 1));
+    auto [min, max] = std::ranges::minmax(range);
+    assert(a[0] == nullptr);
+    assert(min != nullptr);
+    assert(max == min);
+  }
 
   return 0;
 }
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.493778.patch
Type: text/x-patch
Size: 3326 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230201/53fdd14a/attachment.bin>