ldionne wrote: This seems to be a bit stuck. I checked out the patch locally and that was helpful to understand some of the issues. I think the final patch should be:
``` commit c667f036c4228b9cf0172c311537ab96ef61fcf5 Author: PandaNinjas <ad...@malwarefight.wip.la> Date: Wed Sep 13 17:38:17 2023 -0700 [libc++] Prevent calling the projection more than three times Resolves #64717 diff --git a/libcxx/include/__algorithm/ranges_clamp.h b/libcxx/include/__algorithm/ranges_clamp.h index 9613f7f37720..e6c86207254a 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 036552f75de4..35ef55a91e24 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; ``` Can you update the PR accordingly? Also, in the future I believe that following the instructions in https://libcxx.llvm.org/TestingLibcxx.html#usage would make it a lot easier to see issues in testing before running them through the CI. Most of the issues I fixed in the patch locally were pretty immediately visible when running the tests locally. https://github.com/llvm/llvm-project/pull/66315 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits