[clang-tools-extra] [libc++] Fix complexity guarantee in ranges::clamp (PR #68413)
Louis Dionne via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 27 07:12:36 PDT 2023
https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/68413
>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 1/3] [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;
>From 1ba671d6df5e1a13cd824d5abe2731060e332774 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Thu, 26 Oct 2023 10:32:41 -0400
Subject: [PATCH 2/3] Fix clamp test some more
---
.../alg.sorting/alg.clamp/ranges.clamp.pass.cpp | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
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 35ef55a91e243d4..a566412a91b8b37 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
@@ -78,23 +78,15 @@ 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; }
constexpr bool operator==(S const& other) const { return i == other.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; }
- };
-
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);
}
{ // Ensure that we respect the value category of the projection when calling the comparator.
>From 6b79d8b21e841a6e2761758ee81c3aa3da31136b Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Fri, 27 Oct 2023 10:12:18 -0400
Subject: [PATCH 3/3] Fix missing include
---
.../std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp | 1 +
1 file changed, 1 insertion(+)
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 a566412a91b8b37..3d704fe30826fb6 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>
More information about the cfe-commits
mailing list