Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
24d5794
[libc++] Fix complexity guarantee in std::clamp
ldionne Oct 6, 2023
1ba671d
Fix clamp test some more
ldionne Oct 26, 2023
c7bb32e
Merge branch 'main' into review/fix-clamp-complexity
ldionne Oct 26, 2023
6b79d8b
Fix missing include
ldionne Oct 27, 2023
f656911
Fix invalid assertion with debug/safe mode
ldionne Nov 1, 2023
18233d5
Merge branch 'main' into review/fix-clamp-complexity
ldionne Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions libcxx/include/__algorithm/ranges_clamp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would much rather we simply pass the projection down to another function, which ensures the lifetime of the projection, even in weird cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. We're extending the lifetime here with rvalue references, is that what you would want to make more explicit? FWIW, this is how other implementations do it too, and lifetime extending with rvalue references is a fairly common pattern, so if that's what your concern is about my preference would be to keep it simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so if a user passes a projection which happens to implicitly create the projected object in the call to the projection, For example:

std::string const& project_string(std::string const&); // pass "abc"

We currently do accept code that does this, because the returned object is never used outside of the expression which created it. We can keep this behavior simply by creating a __clamp_impl function that takes the projected values as argument and then returns the result. Ex return __clamp_impl( __low, __high, _Comp __comp, std::invoke(__proj, __value), std::invoke(__proj, __low), std::invoke(__proj, __high)); // now the projections live the entire time

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so you are thinking about the following case?

char const* low = "1";
char const* high = "3";
char const* val = "2";
auto projection = [](std::string const& s) -> std::string const& { return s; };
std::ranges::clamp(val, low, high, std::less{}, projection);

Here what you're saying is that the s argument of the projection is created implicitly when we call std::invoke(__proj, __something), and if we end the full-expression before we are done using that result, we'll be using a dangling ref.

That seems to be correct according to the lifetime extension docs on cppreference:

There are following exceptions to this lifetime rule:

  • a temporary bound to a reference parameter in a function call exists until the end of the full expression containing that function call: if the function returns a reference, which outlives the full expression, it becomes a dangling reference.

I think the "problem" I have with that is that it means we could technically never call any user-provided function (projection or else) and hold on to the result of that, since this case could happen anywhere. Every time we call a user-provided function with some arguments, there could be some implicit conversion happening and then we are vulnerable to this dangling reference issue. Am I right? If so, and if we want to guard against this, then we would in theory write all of our code which is potentially vulnerable to this issue in a single full expression. I'm not sure this is either reasonable or expected. WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always dangling no matter how you do it. This isn't __proj(__something), it's std::invoke(__proj, __something). No temporary is created for the call to std::invoke itself, that just binds a bunch of references. The full-expression with the temporary is somewhere inside the std::invoke machinery, and the temporary is destroyed by the time std::invoke returns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh, yes, indeed. So then I don't see a way that we can make this "work" even if we wanted to.

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;
Expand Down
62 changes: 33 additions & 29 deletions libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down