[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 495520.
fsb4000 added a comment.

format


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
@@ -30,6 +30,7 @@
 #include <functional>
 #include <memory>
 #include <ranges>
+#include <string>
 
 #include "test_iterators.h"
 
@@ -324,6 +325,16 @@
     assert(ret.min == 1);
     assert(ret.max == 4);
   }
+  {
+    // check that the input iterator isn't moved from multiple times
+    const std::string str{"this long string will be dynamically allocated"};
+    std::string a[] = {"this long string will be dynamically allocated"};
+    auto range      = std::ranges::subrange(
+        cpp20_input_iterator(std::move_iterator(a)), sentinel_wrapper(cpp20_input_iterator(std::move_iterator(a + 1))));
+    auto [min, max] = std::ranges::minmax(range);
+    assert(min == str);
+    assert(max == str);
+  }
 }
 
 constexpr bool test() {
@@ -339,7 +350,7 @@
   static_assert(test());
 
   {
-    // check that the iterator isn't moved from multiple times
+    // check that the random 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);
Index: libcxx/include/__algorithm/ranges_minmax.h
===================================================================
--- libcxx/include/__algorithm/ranges_minmax.h
+++ libcxx/include/__algorithm/ranges_minmax.h
@@ -13,6 +13,7 @@
 #include <__algorithm/minmax_element.h>
 #include <__assert>
 #include <__concepts/copyable.h>
+#include <__concepts/same_as.h>
 #include <__config>
 #include <__functional/identity.h>
 #include <__functional/invoke.h>
@@ -76,8 +77,20 @@
     _LIBCPP_ASSERT(__first != __last, "range has to contain at least one element");
 
     if constexpr (forward_range<_Range>) {
+      // Special-case the one element case. Avoid repeatedly initializing objects from the result of an iterator
+      // dereference when doing so might not be idempotent. The `if constexpr` avoids the extra branch in cases where
+      // it's not needed.
+      if constexpr (!same_as<remove_cvref_t<range_reference_t<_Range>>, _ValueT> ||
+                    is_rvalue_reference_v<range_reference_t<_Range>>) {
+        auto __test_for_one_element = __first;
+        if (++__test_for_one_element == __last) {
+          // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
+          minmax_result<_ValueT> __result = {static_cast<_ValueT>(*__result.min), __result.min};
+          return __result;
+        }
+      }
       auto __result = std::__minmax_element_impl(__first, __last, __comp, __proj);
-      return {*__result.first, *__result.second};
+      return {static_cast<_ValueT>(*__result.first), static_cast<_ValueT>(*__result.second)};
     } else {
       // 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
@@ -86,7 +99,8 @@
                                    std::invoke(__proj, std::forward<decltype(__b)>(__b)));
       };
 
-      ranges::minmax_result<_ValueT> __result = {*__first, __result.min};
+      // This initialization is correct, similar to the N4928 [dcl.init.aggr]/6 example
+      ranges::minmax_result<_ValueT> __result = {static_cast<_ValueT>(*__first), __result.min};
       if (__first == __last || ++__first == __last)
         return __result;
 


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