Skip to content

[libc++] Make expected trivially assignable whenever possible #47

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libc++] Make expected trivially assignable whenever possible #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 60 additions & 27 deletions libcxx/include/__expected/expected.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/is_trivially_assignable.h>
#include <__type_traits/is_trivially_constructible.h>
#include <__type_traits/is_trivially_destructible.h>
#include <__type_traits/is_trivially_relocatable.h>
Expand Down Expand Up @@ -215,18 +216,24 @@ class __expected_base {
// it's not clear that it's implementable, given that the function is allowed to clobber
// the tail padding) - see https://github.com/itanium-cxx-abi/cxx-abi/issues/107.
union __union_t {
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&)
requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>)
__union_t(const __union_t&) = delete;
_LIBCPP_HIDE_FROM_ABI __union_t(const __union_t&)
requires(is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>)
= default;
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(__union_t&&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __union_t(__union_t&&)
requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&
is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>)
__union_t(__union_t&&) = delete;
_LIBCPP_HIDE_FROM_ABI __union_t(__union_t&&)
requires(is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>)
= default;
__union_t& operator=(const __union_t&) = delete;
_LIBCPP_HIDE_FROM_ABI __union_t& operator=(const __union_t&)
requires(is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err> &&
is_trivially_copy_assignable_v<_Tp> && is_trivially_copy_assignable_v<_Err>)
= default;
__union_t& operator=(__union_t&&) = delete;
_LIBCPP_HIDE_FROM_ABI __union_t& operator=(__union_t&&)
requires(is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err> &&
is_trivially_move_assignable_v<_Tp> && is_trivially_move_assignable_v<_Err>)
= default;
_LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(__union_t&&) = delete;

template <class... _Args>
_LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(in_place_t, _Args&&... __args)
Expand Down Expand Up @@ -293,19 +300,24 @@ class __expected_base {
[&] { return __make_union(__has_val, std::forward<_OtherUnion>(__other)); }),
__has_val_(__has_val) {}

_LIBCPP_HIDE_FROM_ABI constexpr __repr(const __repr&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __repr(const __repr&)
requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>)
__repr(const __repr&) = delete;
_LIBCPP_HIDE_FROM_ABI __repr(const __repr&)
requires is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>
= default;
_LIBCPP_HIDE_FROM_ABI constexpr __repr(__repr&&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __repr(__repr&&)
requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&
is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>)
__repr(__repr&&) = delete;
_LIBCPP_HIDE_FROM_ABI __repr(__repr&&)
requires is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>
= default;
__repr& operator=(const __repr&) = delete;
_LIBCPP_HIDE_FROM_ABI __repr& operator=(const __repr&)
requires is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_assignable_v<_Tp> &&
is_trivially_copy_constructible_v<_Err> && is_trivially_copy_assignable_v<_Err>
= default;
__repr& operator=(__repr&&) = delete;
_LIBCPP_HIDE_FROM_ABI __repr& operator=(__repr&&)
requires is_trivially_move_constructible_v<_Tp> && is_trivially_move_assignable_v<_Tp> &&
is_trivially_move_constructible_v<_Err> && is_trivially_move_assignable_v<_Err>
= default;

_LIBCPP_HIDE_FROM_ABI constexpr __repr& operator=(const __repr&) = delete;
_LIBCPP_HIDE_FROM_ABI constexpr __repr& operator=(__repr&&) = delete;

_LIBCPP_HIDE_FROM_ABI constexpr ~__repr()
requires(is_trivially_destructible_v<_Tp> && is_trivially_destructible_v<_Err>)
Expand Down Expand Up @@ -445,6 +457,19 @@ class __expected_base {
_LIBCPP_NO_UNIQUE_ADDRESS __conditional_no_unique_address<__allow_reusing_expected_tail_padding, __repr> __repr_;
};

template<class _Tp, class _Err>
concept __expected_have_copy_assignment =
is_copy_constructible_v<_Tp> && is_copy_assignable_v<_Tp> &&
is_copy_constructible_v<_Err> && is_copy_assignable_v<_Err> &&
(is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>);

template<class _Tp, class _Err>
concept __expected_have_move_assignment =
is_move_constructible_v<_Tp> && is_move_assignable_v<_Tp> &&
is_move_constructible_v<_Err> && is_move_assignable_v<_Err> &&
(is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>);


template <class _Tp, class _Err>
class expected : private __expected_base<_Tp, _Err> {
static_assert(!is_reference_v<_Tp> && !is_function_v<_Tp> && !is_same_v<remove_cv_t<_Tp>, in_place_t> &&
Expand Down Expand Up @@ -629,9 +654,7 @@ class expected : private __expected_base<_Tp, _Err> {
_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected& __rhs) noexcept(
is_nothrow_copy_assignable_v<_Tp> && is_nothrow_copy_constructible_v<_Tp> && is_nothrow_copy_assignable_v<_Err> &&
is_nothrow_copy_constructible_v<_Err>) // strengthened
requires(is_copy_assignable_v<_Tp> && is_copy_constructible_v<_Tp> && is_copy_assignable_v<_Err> &&
is_copy_constructible_v<_Err> &&
(is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>))
requires __expected_have_copy_assignment<_Tp, _Err>
{
if (this->__has_val() && __rhs.__has_val()) {
this->__val() = __rhs.__val();
Expand All @@ -645,12 +668,16 @@ class expected : private __expected_base<_Tp, _Err> {
return *this;
}

_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected& __rhs)
requires __expected_have_copy_assignment<_Tp, _Err> &&
is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_assignable_v<_Tp> &&
is_trivially_copy_constructible_v<_Err> && is_trivially_copy_assignable_v<_Err>
= default;
Comment on lines +671 to +675
Copy link

Choose a reason for hiding this comment

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

Is it intended not to consider is_trivially_destructible_v? (See also LWG4026 and P0602R4 which considered trivial destructibility.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Today, is_trivially_copy_constructible_v<T> logically implies is_trivially_destructible_v<T>. (Godbolt.) There is some controversy about the desirability of this fact, but it's still a fact. So my personal view is that we shouldn't add redundant conditions that won't ever affect the conjunction's result. But I know that "reasonable people disagree," and I certainly wouldn't object if this were upstreamed with the redundant condition added.

Actually, in Hagenberg this very question came up during discussion of P2786's is_nothrow_relocatable, where the version approved in plenary has ([meta.unary.prop]):

is_trivially_relocatable_v<T> || (is_nothrow_move_constructible_v<remove_all_extents_t<T>> &&
                                  is_nothrow_destructible_v<remove_all_extents_t<T>>)

Today, is_nothrow_move_constructible_v<remove_all_extents_t<T>> logically implies is_nothrow_destructible_v<remove_all_extents_t<T>>, so personally I wouldn't have written it this way. But a vocal contingent on LWG did want the redundant condition, and since it has no effect there's no physical reason to oppose adding it, so, it got added.

Copy link

Choose a reason for hiding this comment

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

Today, is_trivially_copy_constructible_v<T> logically implies is_trivially_destructible_v<T>.

Oh, I think this is arguable, as LWG2116 and LWG2827 aren't resolved yet.

I guess it's worthwhile to explicitly specify that multiple is_trivially_* imply is_trivially_destructible_v (or at least having a trivial non-deleted destructor), as we can hardly do anything special for a trivially-meowable but not trivially destructible class.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Today, is_trivially_copy_constructible_v<T> logically implies is_trivially_destructible_v<T>.

Oh, I think this is arguable, as LWG2116 and LWG2827 aren't resolved yet.

Sure, but LWG did vote in 2022 to close them as NAD/evolutionary (as shown in the minutes for LWG2116); I'm not sure why they haven't been actually closed in the issue system yet.

The evolutionary question is indeed argued. But I don't think such a change stands much chance precisely because there's already code just like the code I wrote, out there in the wild, which would quietly change semantics if we changed the meaning of these traits. In fact, notice that there exists a paper discussing why we might want to change the semantics of these traits (P2842R0, cplusplus/papers#1547 ), and its proposed resolution (§7 and §9) is literally to ratify LWG's position and close the issues as NAD — after all that it's still not proposing that we should change the traits!


_LIBCPP_HIDE_FROM_ABI constexpr expected&
operator=(expected&& __rhs) noexcept(is_nothrow_move_assignable_v<_Tp> && is_nothrow_move_constructible_v<_Tp> &&
is_nothrow_move_assignable_v<_Err> && is_nothrow_move_constructible_v<_Err>)
requires(is_move_constructible_v<_Tp> && is_move_assignable_v<_Tp> && is_move_constructible_v<_Err> &&
is_move_assignable_v<_Err> &&
(is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>))
requires __expected_have_move_assignment<_Tp, _Err>
{
if (this->__has_val() && __rhs.__has_val()) {
this->__val() = std::move(__rhs.__val());
Expand All @@ -664,6 +691,12 @@ class expected : private __expected_base<_Tp, _Err> {
return *this;
}

_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(expected&& __rhs)
requires __expected_have_move_assignment<_Tp, _Err> &&
is_trivially_move_constructible_v<_Tp> && is_trivially_move_assignable_v<_Tp> &&
is_trivially_move_constructible_v<_Err> && is_trivially_move_assignable_v<_Err>
= default;

template <class _Up = _Tp>
_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(_Up&& __v)
requires(!is_same_v<expected, remove_cvref_t<_Up>> && !__is_std_unexpected<remove_cvref_t<_Up>>::value &&
Expand Down
47 changes: 47 additions & 0 deletions libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
// - is_copy_assignable_v<E> is true and
// - is_copy_constructible_v<E> is true and
// - is_nothrow_move_constructible_v<T> || is_nothrow_move_constructible_v<E> is true.
//
// This assignment operator is trivial if
// - is_trivially_copy_constructible_v<T> is true and
// - is_trivially_copy_constructible_v<E> is true and
// - is_trivially_copy_assignable_v<T> is true and
// - is_trivially_copy_assignable_v<E> is true.

#include <cassert>
#include <concepts>
Expand Down Expand Up @@ -55,6 +61,28 @@ struct MoveMayThrow {
MoveMayThrow& operator=(MoveMayThrow&&) noexcept(false) { return *this; }
};

template<int N>
struct CopyableNonTrivial {
int i;
char pad[N];
constexpr CopyableNonTrivial(int ii) : i(ii) {}
constexpr CopyableNonTrivial(const CopyableNonTrivial& o) { i = o.i; }
CopyableNonTrivial(CopyableNonTrivial&&) = default;
constexpr void operator=(const CopyableNonTrivial& o) { i = o.i; }
CopyableNonTrivial& operator=(CopyableNonTrivial&&) = default;
};

template<int N>
struct MovableNonTrivial {
int i;
char pad[N];
constexpr MovableNonTrivial(int ii) : i(ii) {}
MovableNonTrivial(const MovableNonTrivial&) = default;
constexpr MovableNonTrivial(MovableNonTrivial&& o) noexcept { i = o.i; }
MovableNonTrivial& operator=(const MovableNonTrivial&) = default;
constexpr void operator=(MovableNonTrivial&& o) { i = o.i; }
};

// Test constraints
static_assert(std::is_copy_assignable_v<std::expected<int, int>>);

Expand All @@ -79,6 +107,25 @@ static_assert(std::is_copy_assignable_v<std::expected<int, MoveMayThrow>>);
// !is_nothrow_move_constructible_v<T> && !is_nothrow_move_constructible_v<E>
static_assert(!std::is_copy_assignable_v<std::expected<MoveMayThrow, MoveMayThrow>>);

// Test: This constructor is trivial if
// - is_trivially_copy_constructible_v<T> is true and
// - is_trivially_copy_constructible_v<E> is true and
// - is_trivially_copy_assignable_v<T> is true and
// - is_trivially_copy_assignable_v<E> is true.
static_assert(std::is_trivially_copy_assignable_v<std::expected<int, int>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<CopyableNonTrivial<1>, int>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<int, CopyableNonTrivial<1>>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<CopyableNonTrivial<1>, CopyableNonTrivial<1>>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<MovableNonTrivial<1>, int>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<int, MovableNonTrivial<1>>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<MovableNonTrivial<1>, MovableNonTrivial<1>>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<CopyableNonTrivial<4>, int>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<int, CopyableNonTrivial<4>>>);
static_assert(!std::is_trivially_copy_assignable_v<std::expected<CopyableNonTrivial<4>, CopyableNonTrivial<4>>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<MovableNonTrivial<4>, int>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<int, MovableNonTrivial<4>>>);
static_assert(std::is_trivially_copy_assignable_v<std::expected<MovableNonTrivial<4>, MovableNonTrivial<4>>>);

constexpr bool test() {
// If this->has_value() && rhs.has_value() is true, equivalent to val = *rhs.
{
Expand Down
47 changes: 47 additions & 0 deletions libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@
// Remarks: The exception specification is equivalent to:
// is_nothrow_move_assignable_v<T> && is_nothrow_move_constructible_v<T> &&
// is_nothrow_move_assignable_v<E> && is_nothrow_move_constructible_v<E>
//
// This assignment operator is trivial if
// - is_trivially_move_constructible_v<T> is true and
// - is_trivially_move_constructible_v<E> is true and
// - is_trivially_move_assignable_v<T> is true and
// - is_trivially_move_assignable_v<E> is true.

#include <cassert>
#include <concepts>
Expand All @@ -56,6 +62,28 @@ struct MoveCtorMayThrow {
MoveCtorMayThrow& operator=(MoveCtorMayThrow&&) noexcept = default;
};

template<int N>
struct CopyableNonTrivial {
int i;
char pad[N];
constexpr CopyableNonTrivial(int ii) : i(ii) {}
constexpr CopyableNonTrivial(const CopyableNonTrivial& o) { i = o.i; }
CopyableNonTrivial(CopyableNonTrivial&&) = default;
constexpr void operator=(const CopyableNonTrivial& o) { i = o.i; }
CopyableNonTrivial& operator=(CopyableNonTrivial&&) = default;
};

template<int N>
struct MovableNonTrivial {
int i;
char pad[N];
constexpr MovableNonTrivial(int ii) : i(ii) {}
MovableNonTrivial(const MovableNonTrivial&) = default;
constexpr MovableNonTrivial(MovableNonTrivial&& o) noexcept { i = o.i; }
MovableNonTrivial& operator=(const MovableNonTrivial&) = default;
constexpr void operator=(MovableNonTrivial&& o) { i = o.i; }
};

// Test constraints
static_assert(std::is_move_assignable_v<std::expected<int, int>>);

Expand Down Expand Up @@ -100,6 +128,25 @@ static_assert(!std::is_nothrow_move_assignable_v<std::expected<int, MoveAssignMa
// !is_nothrow_move_constructible_v<E>
static_assert(!std::is_nothrow_move_assignable_v<std::expected<int, MoveCtorMayThrow>>);

// Test: This constructor is trivial if
// - is_trivially_move_constructible_v<T> is true and
// - is_trivially_move_constructible_v<E> is true and
// - is_trivially_move_assignable_v<T> is true and
// - is_trivially_move_assignable_v<E> is true.
static_assert(std::is_trivially_move_assignable_v<std::expected<int, int>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<CopyableNonTrivial<1>, int>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<int, CopyableNonTrivial<1>>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<CopyableNonTrivial<1>, CopyableNonTrivial<1>>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<MovableNonTrivial<1>, int>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<int, MovableNonTrivial<1>>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<MovableNonTrivial<1>, MovableNonTrivial<1>>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<CopyableNonTrivial<4>, int>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<int, CopyableNonTrivial<4>>>);
static_assert(std::is_trivially_move_assignable_v<std::expected<CopyableNonTrivial<4>, CopyableNonTrivial<4>>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<MovableNonTrivial<4>, int>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<int, MovableNonTrivial<4>>>);
static_assert(!std::is_trivially_move_assignable_v<std::expected<MovableNonTrivial<4>, MovableNonTrivial<4>>>);

constexpr bool test() {
// If this->has_value() && rhs.has_value() is true, equivalent to val = std::move(*rhs).
{
Expand Down