Skip to content

[libc++] Make sure that __desugars_to isn't tripped up by reference_wrapper, const and ref qualifiers #132092

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 8 commits into from
Mar 25, 2025
Merged

[libc++] Make sure that __desugars_to isn't tripped up by reference_wrapper, const and ref qualifiers #132092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3641b46
[libc++] Make sure that __desugars_to isn't tripped up by reference_w…
ldionne Mar 19, 2025
24f7084
Decouple test from std::equal_to
ldionne Mar 20, 2025
01752d2
Fix modules issues
ldionne Mar 20, 2025
6b152f8
Frozen C++03 headers
ldionne Mar 20, 2025
5a6ea9f
Simplify some uses of __desugars_to with remove_cvref
ldionne Mar 20, 2025
3796483
GCC fixes
ldionne Mar 20, 2025
50a089c
Gcc
ldionne Mar 21, 2025
bfd2a23
Drop volatile
ldionne Mar 24, 2025
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
4 changes: 1 addition & 3 deletions libcxx/include/__algorithm/sort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_trivially_copyable.h>
#include <__type_traits/remove_cvref.h>
#include <__utility/move.h>
#include <__utility/pair.h>
#include <climits>
Expand All @@ -52,8 +51,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Compare, class _Iter, class _Tp = typename iterator_traits<_Iter>::value_type>
inline const bool __use_branchless_sort =
__libcpp_is_contiguous_iterator<_Iter>::value && __is_cheap_to_copy<_Tp> && is_arithmetic<_Tp>::value &&
(__desugars_to_v<__less_tag, __remove_cvref_t<_Compare>, _Tp, _Tp> ||
__desugars_to_v<__greater_tag, __remove_cvref_t<_Compare>, _Tp, _Tp>);
(__desugars_to_v<__less_tag, _Compare, _Tp, _Tp> || __desugars_to_v<__greater_tag, _Compare, _Tp, _Tp>);

namespace __detail {

Expand Down
4 changes: 1 addition & 3 deletions libcxx/include/__algorithm/stable_sort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include <__type_traits/is_integral.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_trivially_assignable.h>
#include <__type_traits/remove_cvref.h>
#include <__utility/move.h>
#include <__utility/pair.h>

Expand Down Expand Up @@ -246,8 +245,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX26 void __stable_sort(
}

#if _LIBCPP_STD_VER >= 17
constexpr auto __default_comp =
__desugars_to_v<__totally_ordered_less_tag, __remove_cvref_t<_Compare>, value_type, value_type >;
constexpr auto __default_comp = __desugars_to_v<__totally_ordered_less_tag, _Compare, value_type, value_type >;
constexpr auto __integral_value =
is_integral_v<value_type > && is_same_v< value_type&, __iter_reference<_RandomAccessIterator>>;
constexpr auto __allowed_radix_sort = __default_comp && __integral_value;
Expand Down
6 changes: 6 additions & 0 deletions libcxx/include/__functional/reference_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <__config>
#include <__functional/weak_result_type.h>
#include <__memory/addressof.h>
#include <__type_traits/desugars_to.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/invoke.h>
#include <__type_traits/is_const.h>
Expand Down Expand Up @@ -149,6 +150,11 @@ void ref(const _Tp&&) = delete;
template <class _Tp>
void cref(const _Tp&&) = delete;

// Let desugars-to pass through std::reference_wrapper
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v<_CanonicalTag, reference_wrapper<_Operation>, _Args...> =
__desugars_to_v<_CanonicalTag, _Operation, _Args...>;

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___FUNCTIONAL_REFERENCE_WRAPPER_H
12 changes: 12 additions & 0 deletions libcxx/include/__type_traits/desugars_to.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,18 @@ struct __totally_ordered_less_tag {};
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v = false;

// For the purpose of determining whether something desugars to something else,
Copy link
Member

Choose a reason for hiding this comment

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

Do we have tests where we test this desugaring with cvref types in our algorithms?

// syntactically, the operation is equivalent to calling `a < b`, and these expressions
// have to be true for any `a` and `b`:
// - `(a < b) == (b > a)`

Here if a and b are a volatile ref to the same object c the statement above may not hold true and can change every time it's tested.

My concern is specifically about volatile.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a subtlety here. This patch only disregards the cv-ref qualifiers on the operation itself, which would be std::less in this case. It does not disregard the cv-ref qualifiers on the arguments that are passed to the function.

Copy link
Member

Choose a reason for hiding this comment

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

Still I'm concerned that a volatile qualified operator is intended not to be optimized. I expect them to be very rare so not desugaring them sounds not a big loss. IMO if we want to desugar them I'd like some tests that desugaring them has no unexpected issues in cases where a volatile operator depends on read and writes not being optimized away.

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 suspect that this is entirely theoretical at the moment, since I don't think we use volatile-qualified predicates anywhere. At least we don't desugar any such predicates that are not stateless, which is where volatile could conceivably make a difference.

I feel that __desugars_to operates at the level of "is this operation mathematically the same as that other operation", not "is this operation exactly the same as that other operation". This distinction is actually important because otherwise we might not want to also strip ref qualifiers from predicates. For the same reason, I don't think we can ever specialize __desugars_to on a predicate that is not stateless, making the volatile qualification issue somewhat moot.

Do you have something concrete in mind like an example of an algorithm that could be passed a volatile predicate and where that could be interpreted as "don't optimize this"?

In the meantime, I think stripping volatile is mostly for consistency and of low value, so I don't mind removing it, but I'd like to either understand a good reason for not doing it, or do it for consistency.

// we disregard const and ref qualifiers on the operation itself.
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v<_CanonicalTag, _Operation const, _Args...> =
__desugars_to_v<_CanonicalTag, _Operation, _Args...>;
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v<_CanonicalTag, _Operation&, _Args...> =
__desugars_to_v<_CanonicalTag, _Operation, _Args...>;
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v<_CanonicalTag, _Operation&&, _Args...> =
__desugars_to_v<_CanonicalTag, _Operation, _Args...>;

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___TYPE_TRAITS_DESUGARS_TO_H
42 changes: 42 additions & 0 deletions libcxx/test/libcxx/type_traits/desugars_to.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME

// This test requires variable templates
// UNSUPPORTED: gcc && c++11

#include <__type_traits/desugars_to.h>

struct Tag {};
struct Operation {};

namespace std {
template <>
bool const __desugars_to_v<Tag, Operation> = true;
}

void tests() {
// Make sure that __desugars_to is false by default
{
struct Foo {};
static_assert(!std::__desugars_to_v<Tag, Foo>, "");
}

// Make sure that __desugars_to bypasses const and ref qualifiers on the operation
{
static_assert(std::__desugars_to_v<Tag, Operation>, ""); // no quals
static_assert(std::__desugars_to_v<Tag, Operation const>, "");

static_assert(std::__desugars_to_v<Tag, Operation&>, "");
static_assert(std::__desugars_to_v<Tag, Operation const&>, "");

static_assert(std::__desugars_to_v<Tag, Operation&&>, "");
static_assert(std::__desugars_to_v<Tag, Operation const&&>, "");
}
}
36 changes: 36 additions & 0 deletions libcxx/test/libcxx/utilities/function.objects/refwrap/desugars_to.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: FROZEN-CXX03-HEADERS-FIXME

// This test requires variable templates
// UNSUPPORTED: gcc && c++11

// <functional>

// reference_wrapper

// Ensure that std::reference_wrapper does not inhibit optimizations based on the
// std::__desugars_to internal helper.

#include <functional>
#include <__type_traits/desugars_to.h>

struct Operation {};
struct Tag {};

namespace std {
template <>
bool const __desugars_to_v<Tag, Operation> = true;
}

static_assert(std::__desugars_to_v<Tag, Operation>, "something is wrong with the test");

// make sure we pass through reference_wrapper
static_assert(std::__desugars_to_v<Tag, std::reference_wrapper<Operation> >, "");
static_assert(std::__desugars_to_v<Tag, std::reference_wrapper<Operation const> >, "");