[libcxx-commits] [libcxx] [libc++] Fix complexity guarantee in std::clamp (PR #68413)

[Louis Dionne via libcxx-commits]

https://github.com/ldionne created https://github.com/llvm/llvm-project/pull/68413

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

Fixes #64717

>From 24d5794c366670fb4d8fe3ec72c27d7c9ef335b9 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Fri, 6 Oct 2023 08:57:23 -0400
Subject: [PATCH] [libc++] Fix complexity guarantee in std::clamp

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

Fixes #64717
---
 libcxx/include/__algorithm/ranges_clamp.h     |  5 +-
 .../alg.clamp/ranges.clamp.pass.cpp           | 49 +++++++++++--------
 2 files changed, 32 insertions(+), 22 deletions(-)

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..35ef55a91e243d4 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
@@ -38,6 +38,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;
@@ -71,6 +81,7 @@ constexpr bool test() {
 
       constexpr const int& lvalue_proj() const { return i; }
       constexpr int prvalue_proj() const { return i; }
+      constexpr bool operator==(S const& other) const { return i == other.i; }
     };
 
     struct Comp {
@@ -82,31 +93,29 @@ constexpr bool test() {
     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);
+    assert(std::ranges::clamp(val, low, high, Comp{}, &S::lvalue_proj) == low);
+    assert(std::ranges::clamp(val, high, low, Comp{}, &S::prvalue_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);
+    assert(counter <= 3);
   }
 
   return true;