From bb20a0d8f8fda540f569ca6dd3bd9a0b4b9491de Mon Sep 17 00:00:00 2001 From: Jonne Ransijn Date: Fri, 11 Apr 2025 16:00:53 +0200 Subject: [PATCH] AK: Allow the `Optional` move assignment operator to be trivial This will change behaviour for moved-from `Optional`s, since they will now no longer clear their value if `T` is trivial. However, a moved-from value should be considered to be in an unspecified state. Use `Optional::clear` or `Optional::release_value` instead. --- AK/Optional.h | 47 ++++++++++++++++++++++++++++++++++++++- Tests/AK/TestOptional.cpp | 40 ++++++++++++++++++++++++++------- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/AK/Optional.h b/AK/Optional.h index 470ce7767ba..f66f785fcd6 100644 --- a/AK/Optional.h +++ b/AK/Optional.h @@ -186,7 +186,7 @@ public: } AK_MAKE_CONDITIONALLY_COPYABLE(Optional, ); - AK_MAKE_CONDITIONALLY_NONMOVABLE(Optional, ); + AK_MAKE_CONDITIONALLY_MOVABLE(Optional, ); AK_MAKE_CONDITIONALLY_DESTRUCTIBLE(Optional, ); ALWAYS_INLINE constexpr Optional(Optional const& other) @@ -250,7 +250,34 @@ public: return *this; } + Optional& operator=(Optional&& other) + requires(!IsMoveConstructible || !IsDestructible) + = delete; + + // Note: This overload is optional. It exists purely to match the SerenityOS and `std::optional` behaviour. + // The only (observable) difference between this overload and the next one is that this one calls the move assignment operator when both `this` and `other` have a value. + // The other overload just unconditionally calls the move constructor. ALWAYS_INLINE constexpr Optional& operator=(Optional&& other) + requires(IsMoveAssignable && IsMoveConstructible && (!IsTriviallyMoveAssignable || !IsTriviallyMoveConstructible || !IsTriviallyDestructible)) + { + if (this != &other) { + if (has_value() && other.has_value()) { + value() = other.release_value(); + } else if (has_value()) { + value().~T(); + m_has_value = false; + } else if (other.has_value()) { + m_has_value = true; + construct_at>(&m_storage, other.release_value()); + } + } + return *this; + } + + // Allow for move constructible but non-move assignable types, such as those containing const or reference fields, + // Note: This overload can also handle move assignable types perfectly fine, but the behaviour would be slightly different. + ALWAYS_INLINE constexpr Optional& operator=(Optional&& other) + requires(!IsMoveAssignable && IsMoveConstructible && (!IsTriviallyMoveConstructible || !IsTriviallyDestructible)) { if (this != &other) { clear(); @@ -261,6 +288,24 @@ public: return *this; } + template + requires(!IsOneOfIgnoringCVReference, OptionalNone> && !(IsSame && IsScalar)) + // Note: We restrict this to `!IsScalar` to prevent undesired overload resolution for `= {}`. + ALWAYS_INLINE constexpr Optional& operator=(U&& value) + requires(IsConstructible) + { + if constexpr (IsAssignable, AddRvalueReference>) { + if (m_has_value) + m_storage = forward(value); + else + construct_at>(&m_storage, forward(value)); + m_has_value = true; + } else { + emplace(forward(value)); + } + return *this; + } + ALWAYS_INLINE constexpr ~Optional() requires(!IsTriviallyDestructible && IsDestructible) { diff --git a/Tests/AK/TestOptional.cpp b/Tests/AK/TestOptional.cpp index 77f7944ffc8..3773eec0282 100644 --- a/Tests/AK/TestOptional.cpp +++ b/Tests/AK/TestOptional.cpp @@ -67,7 +67,6 @@ TEST_CASE(move_optional) y = move(x); EXPECT_EQ(y.has_value(), true); EXPECT_EQ(y.value(), 3); - EXPECT_EQ(x.has_value(), false); } TEST_CASE(optional_rvalue_ref_qualified_getters) @@ -194,17 +193,44 @@ TEST_CASE(test_constexpr) static_assert(!(Optional { 1 } = {}).has_value(), "Assigning a `{}` should clear the Optional, even for scalar types^^"); } +TEST_CASE(non_trivial_destructor_is_called_on_move_assignment) +{ + static int foo_destruction_count = 0; + + struct Foo { + Foo() { } + Foo(Foo&&) = default; + ~Foo() + { + ++foo_destruction_count; + } + + Foo& operator=(Foo&&) = default; + }; + static_assert(!IsTriviallyMoveAssignable>); + + Optional foo = Foo {}; // 1. The immediate value needs to be destroyed + Optional foo2; + foo = AK::move(foo2); // 2. The move releases the value, which destroys the moved-from stored value + + EXPECT_EQ(foo_destruction_count, 2); + + // As Optional does not trivially move, moved-from values are empty + // Ignoring the fact that we are touching a moved from value here + EXPECT_EQ(foo.has_value(), false); +} + TEST_CASE(test_copy_ctor_and_dtor_called) { -#ifdef AK_HAVE_CONDITIONALLY_TRIVIAL static_assert(IsTriviallyDestructible>); static_assert(IsTriviallyCopyable>); static_assert(IsTriviallyCopyConstructible>); static_assert(IsTriviallyCopyAssignable>); - // These can't be trivial as we have to clear the original object. - static_assert(!IsTriviallyMoveConstructible>); - static_assert(!IsTriviallyMoveAssignable>); -#endif + static_assert(IsTriviallyMoveConstructible>); + static_assert(IsTriviallyMoveAssignable>); + static_assert(IsTriviallyCopyConstructible>); + static_assert(IsTriviallyCopyAssignable>); + static_assert(IsTriviallyDestructible>); struct DestructionChecker { explicit DestructionChecker(bool& was_destroyed) @@ -278,12 +304,10 @@ TEST_CASE(test_copy_ctor_and_dtor_called) Optional move2 = move(move1); EXPECT(was_moved); -#ifdef AK_HAVE_CONDITIONALLY_TRIVIAL struct NonDestructible { ~NonDestructible() = delete; }; static_assert(!IsDestructible>); -#endif } TEST_CASE(basic_optional_reference)