[libcxx] makes expected trivially assignable when both members are …
#74768
Conversation
…trivially assignable
cjdb
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Christopher Di Bella (cjdb) Changes…trivially assignable Full diff: https://github.com/llvm/llvm-project/pull/74768.diff 3 Files Affected:
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index bf16c8f720d26..128369fc1b0a1 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -38,8 +38,10 @@
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
+#include <__type_traits/is_trivially_copy_assignable.h>
#include <__type_traits/is_trivially_copy_constructible.h>
#include <__type_traits/is_trivially_destructible.h>
+#include <__type_traits/is_trivially_move_assignable.h>
#include <__type_traits/is_trivially_move_constructible.h>
#include <__type_traits/is_void.h>
#include <__type_traits/lazy.h>
@@ -282,10 +284,27 @@ class expected {
}
}
+ static constexpr bool __is_trivially_move_assignable =
+ is_trivially_move_constructible_v<_Tp> &&
+ is_trivially_move_assignable_v<_Tp> &&
+ is_trivially_move_constructible_v<_Err> &&
+ is_trivially_move_assignable_v<_Err>;
+
+ static constexpr bool __is_trivially_copy_assignable =
+ __is_trivially_move_assignable &&
+ is_trivially_copy_constructible_v<_Tp> &&
+ is_trivially_copy_assignable_v<_Tp> &&
+ is_trivially_copy_constructible_v<_Err> &&
+ is_trivially_copy_assignable_v<_Err>;
+
public:
// [expected.object.assign], assignment
_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected& __rhs)
+ requires __is_trivially_copy_assignable
+ = default;
+
_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected& __rhs)
noexcept(is_nothrow_copy_assignable_v<_Tp> &&
is_nothrow_copy_constructible_v<_Tp> &&
@@ -295,6 +314,7 @@ class expected {
is_copy_constructible_v<_Tp> &&
is_copy_assignable_v<_Err> &&
is_copy_constructible_v<_Err> &&
+ !__is_trivially_copy_assignable &&
(is_nothrow_move_constructible_v<_Tp> ||
is_nothrow_move_constructible_v<_Err>))
{
@@ -312,6 +332,10 @@ class expected {
return *this;
}
+ _LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(expected&& __rhs)
+ requires __is_trivially_move_assignable
+ = default;
+
_LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(expected&& __rhs)
noexcept(is_nothrow_move_assignable_v<_Tp> &&
is_nothrow_move_constructible_v<_Tp> &&
@@ -321,6 +345,7 @@ class expected {
is_move_assignable_v<_Tp> &&
is_move_constructible_v<_Err> &&
is_move_assignable_v<_Err> &&
+ !__is_trivially_move_assignable &&
(is_nothrow_move_constructible_v<_Tp> ||
is_nothrow_move_constructible_v<_Err>))
{
diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp
index 03fe888b0a5e7..12ca07a3c1f9a 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp
@@ -47,6 +47,22 @@ struct NotCopyAssignable {
NotCopyAssignable& operator=(const NotCopyAssignable&) = delete;
};
+struct NotTriviallyCopyConstructible {
+ NotTriviallyCopyConstructible(const NotTriviallyCopyConstructible&);
+ NotTriviallyCopyConstructible(NotTriviallyCopyConstructible&&) = default;
+
+ NotTriviallyCopyConstructible& operator=(const NotTriviallyCopyConstructible&) = default;
+ NotTriviallyCopyConstructible& operator=(NotTriviallyCopyConstructible&&) = default;
+};
+
+struct NotTriviallyCopyAssignable {
+ NotTriviallyCopyAssignable(const NotTriviallyCopyAssignable&) = default;
+ NotTriviallyCopyAssignable(NotTriviallyCopyAssignable&&) = default;
+
+ NotTriviallyCopyAssignable& operator=(const NotTriviallyCopyAssignable&);
+ NotTriviallyCopyAssignable& operator=(NotTriviallyCopyAssignable&&) = default;
+};
+
struct MoveMayThrow {
MoveMayThrow(MoveMayThrow const&) = default;
MoveMayThrow& operator=(const MoveMayThrow&) = default;
@@ -55,7 +71,7 @@ struct MoveMayThrow {
};
// Test constraints
-static_assert(std::is_copy_assignable_v<std::expected<int, int>>);
+static_assert(std::is_trivially_copy_assignable_v<std::expected<int, int>>);
// !is_copy_assignable_v<T>
static_assert(!std::is_copy_assignable_v<std::expected<NotCopyAssignable, int>>);
@@ -78,6 +94,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>>);
+// !is_trivially_copy_constructible
+static_assert(std::is_copy_assignable_v<std::expected<NotTriviallyCopyConstructible, int>>);
+static_assert(!std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyConstructible, int>>);
+static_assert(!std::is_trivially_copy_assignable_v<std::expected<int, NotTriviallyCopyConstructible>>);
+static_assert(
+ !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyConstructible, NotTriviallyCopyConstructible>>);
+
+// !is_trivially_copy_assignable
+static_assert(std::is_copy_assignable_v<std::expected<NotTriviallyCopyAssignable, int>>);
+static_assert(!std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyAssignable, int>>);
+static_assert(!std::is_trivially_copy_assignable_v<std::expected<int, NotTriviallyCopyAssignable>>);
+static_assert(
+ !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyAssignable, NotTriviallyCopyAssignable>>);
+
+static_assert(
+ !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyConstructible, NotTriviallyCopyAssignable>>);
+static_assert(
+ !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyAssignable, NotTriviallyCopyConstructible>>);
+
constexpr bool test() {
// If this->has_value() && rhs.has_value() is true, equivalent to val = *rhs.
{
diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp
index 8c419afd10729..b08023b9c95ef 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp
@@ -50,13 +50,29 @@ struct NotMoveAssignable {
NotMoveAssignable& operator=(NotMoveAssignable&&) = delete;
};
+struct NotTriviallyMoveConstructible {
+ NotTriviallyMoveConstructible(const NotTriviallyMoveConstructible&) = default;
+ NotTriviallyMoveConstructible(NotTriviallyMoveConstructible&&);
+
+ NotTriviallyMoveConstructible& operator=(const NotTriviallyMoveConstructible&) = default;
+ NotTriviallyMoveConstructible& operator=(NotTriviallyMoveConstructible&&) = default;
+};
+
+struct NotTriviallyMoveAssignable {
+ NotTriviallyMoveAssignable(const NotTriviallyMoveAssignable&) = default;
+ NotTriviallyMoveAssignable(NotTriviallyMoveAssignable&&) = default;
+
+ NotTriviallyMoveAssignable& operator=(const NotTriviallyMoveAssignable&) = default;
+ NotTriviallyMoveAssignable& operator=(NotTriviallyMoveAssignable&&);
+};
+
struct MoveCtorMayThrow {
MoveCtorMayThrow(MoveCtorMayThrow&&) noexcept(false) {}
MoveCtorMayThrow& operator=(MoveCtorMayThrow&&) noexcept = default;
};
// Test constraints
-static_assert(std::is_move_assignable_v<std::expected<int, int>>);
+static_assert(std::is_trivially_move_assignable_v<std::expected<int, int>>);
// !is_move_assignable_v<T>
static_assert(!std::is_move_assignable_v<std::expected<NotMoveAssignable, int>>);
@@ -99,6 +115,21 @@ 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>>);
+// !is_trivially_move_constructible
+static_assert(std::is_move_assignable_v<std::expected<NotTriviallyMoveConstructible, int>>);
+static_assert(!std::is_trivially_move_assignable_v<std::expected<NotTriviallyMoveConstructible, int>>);
+static_assert(!std::is_trivially_move_assignable_v<std::expected<int, NotTriviallyMoveConstructible>>);
+
+// !is_trivially_move_assignable
+static_assert(std::is_move_assignable_v<std::expected<NotTriviallyMoveAssignable, int>>);
+static_assert(!std::is_trivially_move_assignable_v<std::expected<NotTriviallyMoveAssignable, int>>);
+static_assert(!std::is_trivially_move_assignable_v<std::expected<int, NotTriviallyMoveAssignable>>);
+
+static_assert(
+ !std::is_trivially_move_assignable_v<std::expected<NotTriviallyMoveConstructible, NotTriviallyMoveAssignable>>);
+static_assert(
+ !std::is_trivially_move_assignable_v<std::expected<NotTriviallyMoveAssignable, NotTriviallyMoveConstructible>>);
+
constexpr bool test() {
// If this->has_value() && rhs.has_value() is true, equivalent to val = std::move(*rhs).
{
|
You can test this locally with the following command:git-clang-format --diff c79f94d85121347d28f894d837f173f90f368e92 d9ffc415f43e2a05b6d55483709b882adb34ccc6 -- libcxx/include/__expected/expected.h libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cppView the diff from clang-format here.diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 128369fc1b..3d5fef5952 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -285,17 +285,12 @@ private:
}
static constexpr bool __is_trivially_move_assignable =
- is_trivially_move_constructible_v<_Tp> &&
- is_trivially_move_assignable_v<_Tp> &&
- is_trivially_move_constructible_v<_Err> &&
- is_trivially_move_assignable_v<_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>;
static constexpr bool __is_trivially_copy_assignable =
- __is_trivially_move_assignable &&
- is_trivially_copy_constructible_v<_Tp> &&
- is_trivially_copy_assignable_v<_Tp> &&
- is_trivially_copy_constructible_v<_Err> &&
- is_trivially_copy_assignable_v<_Err>;
+ __is_trivially_move_assignable && is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_assignable_v<_Tp> &&
+ is_trivially_copy_constructible_v<_Err> && is_trivially_copy_assignable_v<_Err>;
public:
// [expected.object.assign], assignment
@@ -305,18 +300,12 @@ public:
requires __is_trivially_copy_assignable
= default;
- _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_trivially_copy_assignable &&
- (is_nothrow_move_constructible_v<_Tp> ||
- is_nothrow_move_constructible_v<_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_trivially_copy_assignable &&
+ (is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>))
{
if (__has_val_ && __rhs.__has_val_) {
__union_.__val_ = __rhs.__union_.__val_;
@@ -336,18 +325,12 @@ public:
requires __is_trivially_move_assignable
= default;
- _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_trivially_move_assignable &&
- (is_nothrow_move_constructible_v<_Tp> ||
- is_nothrow_move_constructible_v<_Err>))
+ _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_trivially_move_assignable &&
+ (is_nothrow_move_constructible_v<_Tp> || is_nothrow_move_constructible_v<_Err>))
{
if (__has_val_ && __rhs.__has_val_) {
__union_.__val_ = std::move(__rhs.__union_.__val_);
|
| is_trivially_move_constructible_v<_Err> && | ||
| is_trivially_move_assignable_v<_Err>; | ||
|
|
||
| static constexpr bool __is_trivially_copy_assignable = |
There was a problem hiding this comment.
This is incorrect. The types’ assignment operators were not used at all.
If you assign an expected with value to an expected with error, what’s in the spec is to destroy + construct.
so what you really need to check is the destructor and copy/move constructor.
There was a problem hiding this comment.
The conditions should be consistent with those of variant: [variant.assign]/5, [variant.assign]/10.
| !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyConstructible, NotTriviallyCopyAssignable>>); | ||
| static_assert( | ||
| !std::is_trivially_copy_assignable_v<std::expected<NotTriviallyCopyAssignable, NotTriviallyCopyConstructible>>); | ||
|
|
There was a problem hiding this comment.
Please add a runtime test to make sure the values after assignment are correct.
|
FWIW I've submitted LWG4026 for this. |
|
I will tentatively close this to clean up the review queue, let's reopen once we have a resolution to the LWG issue. |
|
Hmm, I went too fast here. we'd be allowed to make this change as a QOI matter regardless of what the spec says. |
|
@cjdb It would be great to land this before the release branch to get the ABI breaks into a single release. |
I actually considered the ABI aspect of this before, and I concluded that this wasn't ABI breaking since only trivial constructibility is taken into account for being trivial-for-the-purpose-of-ABI. From the Itanium C++ ABI:
Am I wrong? If this is an ABI break, we have to consider it way more carefully than I thought and TBH I think the LWG issue will probably come back as NAD. |
It doesn't necessarily break calls, but it breaks the ABI in the sense that the layout of other types can change if the result of |
|
It might be worthwhile to take @huixie90 's suggestion from the earlier PR into account to add another " |
…trivially assignable