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

[Igor Zhukov via libcxx-commits]

Author: Igor Zhukov
Date: 2023-02-15T06:02:12+07:00
New Revision: 0a0d58f5465a2524ec9ed10ed0bb277cab5a180c

URL: https://github.com/llvm/llvm-project/commit/0a0d58f5465a2524ec9ed10ed0bb277cab5a180c
DIFF: https://github.com/llvm/llvm-project/commit/0a0d58f5465a2524ec9ed10ed0bb277cab5a180c.diff

LOG: [libc++] `<algorithm>`: `ranges::minmax` should dereference iterators only once

Reviewed By: philnik, #libc

Differential Revision: https://reviews.llvm.org/D142864

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__algorithm/ranges_minmax.h b/libcxx/include/__algorithm/ranges_minmax.h
index ea6e54ce016a0..abbe2d41d2ac1 100644
--- a/libcxx/include/__algorithm/ranges_minmax.h
+++ b/libcxx/include/__algorithm/ranges_minmax.h
@@ -13,11 +13,13 @@
 #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>
 #include <__functional/ranges_operations.h>
 #include <__iterator/concepts.h>
+#include <__iterator/next.h>
 #include <__iterator/projected.h>
 #include <__ranges/access.h>
 #include <__ranges/concepts.h>
@@ -76,6 +78,18 @@ struct __fn {
     _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>>) {
+        if (ranges::next(__first) == __last) {
+          // During initialization, members are allowed to refer to already initialized members
+          // (see http://eel.is/c++draft/dcl.init.aggr#6)
+          minmax_result<_ValueT> __result = {*__first, __result.min};
+          return __result;
+        }
+      }
       auto __result = std::__minmax_element_impl(__first, __last, __comp, __proj);
       return {*__result.first, *__result.second};
     } else {
@@ -86,6 +100,8 @@ struct __fn {
                                    std::invoke(__proj, std::forward<decltype(__b)>(__b)));
       };
 
+      // During initialization, members are allowed to refer to already initialized members
+      // (see http://eel.is/c++draft/dcl.init.aggr#6)
       ranges::minmax_result<_ValueT> __result = {*__first, __result.min};
       if (__first == __last || ++__first == __last)
         return __result;

diff  --git a/libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp
index 828a820acce99..fd0d6dfc601c8 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.min.max/ranges.minmax.pass.cpp
+++ b/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,26 @@ constexpr void test_range() {
     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[] = {str};
+    auto range      = std::ranges::subrange(
+        cpp20_input_iterator(std::move_iterator(a)), sentinel_wrapper(cpp20_input_iterator(std::move_iterator(a + 1))));
+    auto ret = std::ranges::minmax(range);
+    assert(ret.min == str);
+    assert(ret.max == str);
+  }
+  {
+    // check that the forward iterator isn't moved from multiple times
+    const std::string str{"this long string will be dynamically allocated"};
+    std::string a[] = {str};
+    auto range =
+        std::ranges::subrange(forward_iterator(std::move_iterator(a)), forward_iterator(std::move_iterator(a + 1)));
+    auto ret = std::ranges::minmax(range);
+    assert(ret.min == str);
+    assert(ret.max == str);
+  }
 }
 
 constexpr bool test() {
@@ -338,15 +359,5 @@ int main(int, char**) {
   test();
   static_assert(test());
 
-  {
-    // check that the 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);
-  }
-
   return 0;
 }