[libcxx-commits] [libcxx] 979c19a - [libc++] Fix complexity guarantee in ranges::clamp (#68413)

[via libcxx-commits]

Author: Louis Dionne
Date: 2023-11-01T10:43:05-04:00
New Revision: 979c19ab12f1a6defd9436fc8eaf2bca35fbd655

URL: https://github.com/llvm/llvm-project/commit/979c19ab12f1a6defd9436fc8eaf2bca35fbd655
DIFF: https://github.com/llvm/llvm-project/commit/979c19ab12f1a6defd9436fc8eaf2bca35fbd655.diff

LOG: [libc++] Fix complexity guarantee in ranges::clamp (#68413)

This patch prevents us from calling the projection more than 3 times in
std::clamp, as required by the Standard.

Fixes #64717

Added: 
    

Modified: 
    libcxx/include/__algorithm/ranges_clamp.h
    libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__algorithm/ranges_clamp.h b/libcxx/include/__algorithm/ranges_clamp.h
index 9613f7f37720a6c..e6c86207254a19f 100644
--- a/libcxx/include/__algorithm/ranges_clamp.h
+++ b/libcxx/include/__algorithm/ranges_clamp.h
@@ -37,9 +37,10 @@ struct __fn {
     _LIBCPP_ASSERT_UNCATEGORIZED(!bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
                                  "Bad bounds passed to std::ranges::clamp");
 
-    if (std::invoke(__comp, std::invoke(__proj, __value), std::invoke(__proj, __low)))
+    auto&& __projected = std::invoke(__proj, __value);
+    if (std::invoke(__comp, std::forward<decltype(__projected)>(__projected), std::invoke(__proj, __low)))
       return __low;
-    else if (std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __value)))
+    else if (std::invoke(__comp, std::invoke(__proj, __high), std::forward<decltype(__projected)>(__projected)))
       return __high;
     else
       return __value;

diff  --git a/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
index 036552f75de4eb4..9dd749cf49619f7 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
@@ -19,6 +19,7 @@
 #include <cassert>
 #include <concepts>
 #include <functional>
+#include <iterator>
 #include <utility>
 
 template <class T, class Comp = std::ranges::less, class Proj = std::identity>
@@ -38,6 +39,16 @@ static_assert(!HasClamp<NoComp>);
 static_assert(!HasClamp<int, NoComp>);
 static_assert(!HasClamp<int, std::ranges::less, CreateNoComp>);
 
+struct EnsureValueCategoryComp {
+  constexpr bool operator()(const int&& x, const int&& y) const { return x < y; }
+  constexpr bool operator()(const int&& x, int& y) const { return x < y; }
+  constexpr bool operator()(int& x, const int&& y) const { return x < y; }
+  constexpr bool operator()(int& x, int& y) const { return x < y; }
+  constexpr bool operator()(std::same_as<const int&> auto&& x, std::same_as<const int&> auto&& y) const {
+    return x < y;
+  }
+};
+
 constexpr bool test() {
   { // low < val < high
     int val = 2;
@@ -68,45 +79,38 @@ constexpr bool test() {
   { // Check that a custom projection works.
     struct S {
       int i;
-
-      constexpr const int& lvalue_proj() const { return i; }
-      constexpr int prvalue_proj() const { return i; }
-    };
-
-    struct Comp {
-      constexpr bool operator()(const int& lhs, const int& rhs) const { return lhs < rhs; }
-      constexpr bool operator()(int&& lhs, int&& rhs) const { return lhs > rhs; }
+      constexpr bool operator==(S const& other) const { return i == other.i; }
     };
 
     auto val = S{10};
     auto low = S{20};
     auto high = S{30};
-    // Check that the value category of the projection return type is preserved.
-    assert(&std::ranges::clamp(val, low, high, Comp{}, &S::lvalue_proj) == &low);
-    assert(&std::ranges::clamp(val, high, low, Comp{}, &S::prvalue_proj) == &low);
+    auto proj = [](S const& s) -> int const& { return s.i; };
+
+    assert(std::ranges::clamp(val, low, high, std::less{}, proj) == low);
   }
 
-  { // Check that the implementation doesn't cause double moves (which could result from calling the projection on
-    // `value` once and then forwarding the result into the comparator).
-    struct CheckDoubleMove {
-      int i;
-      bool moved = false;
-
-      constexpr explicit CheckDoubleMove(int set_i) : i(set_i) {}
-      constexpr CheckDoubleMove(const CheckDoubleMove&) = default;
-      constexpr CheckDoubleMove(CheckDoubleMove&& rhs) noexcept : i(rhs.i) {
-        assert(!rhs.moved);
-        rhs.moved = true;
-      }
+  { // Ensure that we respect the value category of the projection when calling the comparator.
+    // This additional example was provided by Tim Song in https://github.com/microsoft/STL/issues/3970#issuecomment-1685120958.
+    struct MoveProj {
+      constexpr int const&& operator()(int const& x) const { return std::move(x); }
     };
 
-    auto val = CheckDoubleMove{20};
-    auto low = CheckDoubleMove{10};
-    auto high = CheckDoubleMove{30};
+    static_assert(std::indirect_strict_weak_order<EnsureValueCategoryComp, std::projected<const int*, MoveProj>>);
 
-    auto moving_comp = [](CheckDoubleMove lhs, CheckDoubleMove rhs) { return lhs.i < rhs.i; };
-    auto prvalue_proj = [](const CheckDoubleMove& x) -> CheckDoubleMove { return x; };
-    assert(&std::ranges::clamp(val, low, high, moving_comp, prvalue_proj) == &val);
+    assert(std::ranges::clamp(0, 1, 2, EnsureValueCategoryComp{}, MoveProj{}) == 1);
+  }
+
+  { // Make sure we don't call the projection more than three times per [alg.clamp], see #64717
+    int counter              = 0;
+    auto projection_function = [&counter](const int value) -> int {
+      counter++;
+      return value;
+    };
+    assert(std::ranges::clamp(3, 2, 4, std::ranges::less{}, projection_function) == 3);
+#if !(defined(_LIBCPP_ENABLE_SAFE_MODE) || defined(_LIBCPP_ENABLE_DEBUG_MODE))
+    assert(counter <= 3);
+#endif
   }
 
   return true;