Skip to content

Commit 979c19a

Browse files
authored
[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
1 parent 8c0ec79 commit 979c19a

File tree

2 files changed

+36
-31
lines changed

2 files changed

+36
-31
lines changed

libcxx/include/__algorithm/ranges_clamp.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,10 @@ struct __fn {
3737
_LIBCPP_ASSERT_UNCATEGORIZED(!bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
3838
"Bad bounds passed to std::ranges::clamp");
3939

40-
if (std::invoke(__comp, std::invoke(__proj, __value), std::invoke(__proj, __low)))
40+
auto&& __projected = std::invoke(__proj, __value);
41+
if (std::invoke(__comp, std::forward<decltype(__projected)>(__projected), std::invoke(__proj, __low)))
4142
return __low;
42-
else if (std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __value)))
43+
else if (std::invoke(__comp, std::invoke(__proj, __high), std::forward<decltype(__projected)>(__projected)))
4344
return __high;
4445
else
4546
return __value;

libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp

Lines changed: 33 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include <cassert>
2020
#include <concepts>
2121
#include <functional>
22+
#include <iterator>
2223
#include <utility>
2324

2425
template <class T, class Comp = std::ranges::less, class Proj = std::identity>
@@ -38,6 +39,16 @@ static_assert(!HasClamp<NoComp>);
3839
static_assert(!HasClamp<int, NoComp>);
3940
static_assert(!HasClamp<int, std::ranges::less, CreateNoComp>);
4041

42+
struct EnsureValueCategoryComp {
43+
constexpr bool operator()(const int&& x, const int&& y) const { return x < y; }
44+
constexpr bool operator()(const int&& x, int& y) const { return x < y; }
45+
constexpr bool operator()(int& x, const int&& y) const { return x < y; }
46+
constexpr bool operator()(int& x, int& y) const { return x < y; }
47+
constexpr bool operator()(std::same_as<const int&> auto&& x, std::same_as<const int&> auto&& y) const {
48+
return x < y;
49+
}
50+
};
51+
4152
constexpr bool test() {
4253
{ // low < val < high
4354
int val = 2;
@@ -68,45 +79,38 @@ constexpr bool test() {
6879
{ // Check that a custom projection works.
6980
struct S {
7081
int i;
71-
72-
constexpr const int& lvalue_proj() const { return i; }
73-
constexpr int prvalue_proj() const { return i; }
74-
};
75-
76-
struct Comp {
77-
constexpr bool operator()(const int& lhs, const int& rhs) const { return lhs < rhs; }
78-
constexpr bool operator()(int&& lhs, int&& rhs) const { return lhs > rhs; }
82+
constexpr bool operator==(S const& other) const { return i == other.i; }
7983
};
8084

8185
auto val = S{10};
8286
auto low = S{20};
8387
auto high = S{30};
84-
// Check that the value category of the projection return type is preserved.
85-
assert(&std::ranges::clamp(val, low, high, Comp{}, &S::lvalue_proj) == &low);
86-
assert(&std::ranges::clamp(val, high, low, Comp{}, &S::prvalue_proj) == &low);
88+
auto proj = [](S const& s) -> int const& { return s.i; };
89+
90+
assert(std::ranges::clamp(val, low, high, std::less{}, proj) == low);
8791
}
8892

89-
{ // Check that the implementation doesn't cause double moves (which could result from calling the projection on
90-
// `value` once and then forwarding the result into the comparator).
91-
struct CheckDoubleMove {
92-
int i;
93-
bool moved = false;
94-
95-
constexpr explicit CheckDoubleMove(int set_i) : i(set_i) {}
96-
constexpr CheckDoubleMove(const CheckDoubleMove&) = default;
97-
constexpr CheckDoubleMove(CheckDoubleMove&& rhs) noexcept : i(rhs.i) {
98-
assert(!rhs.moved);
99-
rhs.moved = true;
100-
}
93+
{ // Ensure that we respect the value category of the projection when calling the comparator.
94+
// This additional example was provided by Tim Song in https://github.com/microsoft/STL/issues/3970#issuecomment-1685120958.
95+
struct MoveProj {
96+
constexpr int const&& operator()(int const& x) const { return std::move(x); }
10197
};
10298

103-
auto val = CheckDoubleMove{20};
104-
auto low = CheckDoubleMove{10};
105-
auto high = CheckDoubleMove{30};
99+
static_assert(std::indirect_strict_weak_order<EnsureValueCategoryComp, std::projected<const int*, MoveProj>>);
106100

107-
auto moving_comp = [](CheckDoubleMove lhs, CheckDoubleMove rhs) { return lhs.i < rhs.i; };
108-
auto prvalue_proj = [](const CheckDoubleMove& x) -> CheckDoubleMove { return x; };
109-
assert(&std::ranges::clamp(val, low, high, moving_comp, prvalue_proj) == &val);
101+
assert(std::ranges::clamp(0, 1, 2, EnsureValueCategoryComp{}, MoveProj{}) == 1);
102+
}
103+
104+
{ // Make sure we don't call the projection more than three times per [alg.clamp], see #64717
105+
int counter = 0;
106+
auto projection_function = [&counter](const int value) -> int {
107+
counter++;
108+
return value;
109+
};
110+
assert(std::ranges::clamp(3, 2, 4, std::ranges::less{}, projection_function) == 3);
111+
#if !(defined(_LIBCPP_ENABLE_SAFE_MODE) || defined(_LIBCPP_ENABLE_DEBUG_MODE))
112+
assert(counter <= 3);
113+
#endif
110114
}
111115

112116
return true;

0 commit comments

Comments
 (0)