From e86eda622282fb1898305f1aa430171c390075dd Mon Sep 17 00:00:00 2001 From: Pranjal Raihan Date: Wed, 18 Sep 2024 12:06:20 -0700 Subject: [PATCH] Make expected support movable and non-movable types + add tests Summary: The code already pretty much worked except for `reinit_impl` which had a bug that was never exercised previously. Reviewed By: vitaut Differential Revision: D62713190 fbshipit-source-id: ff7e5fd038c44c601c3cde60cad34f51947f2afa --- thrift/compiler/whisker/expected.h | 20 +++-- thrift/compiler/whisker/test/expected_test.cc | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+), 7 deletions(-) diff --git a/thrift/compiler/whisker/expected.h b/thrift/compiler/whisker/expected.h index fbd9a2a4e56..7f2ea79d480 100644 --- a/thrift/compiler/whisker/expected.h +++ b/thrift/compiler/whisker/expected.h @@ -215,12 +215,8 @@ template class expected { static_assert(detail::check_valid_value_type()); static_assert(!std::is_void_v, "Use std::monostate instead."); - static_assert(std::is_nothrow_move_constructible_v); - static_assert(std::is_nothrow_move_assignable_v); static_assert(detail::check_valid_error_type()); - static_assert(std::is_nothrow_move_constructible_v); - static_assert(std::is_nothrow_move_assignable_v); // Restrictions on constructor (6): // https://en.cppreference.com/w/cpp/utility/expected/expected#Version_6 @@ -296,12 +292,22 @@ class expected { expected(const expected& other) noexcept( std::is_nothrow_copy_constructible_v && std::is_nothrow_copy_constructible_v) - : storage_(from_other(other.storage_)) {} + : storage_(from_other(other.storage_)) { + // To properly SFINAE this, we need to move the copy ctor to a base class or + // use C++20 concepts + static_assert( + std::is_copy_constructible_v && std::is_copy_constructible_v); + } // https://en.cppreference.com/w/cpp/utility/expected/expected#Version_3 expected(expected&& other) noexcept( std::is_nothrow_move_constructible_v && std::is_nothrow_move_constructible_v) - : storage_(from_other(std::move(other.storage_))) {} + : storage_(from_other(std::move(other.storage_))) { + // To properly SFINAE this, we need to move the move ctor to a base class or + // use C++20 concepts + static_assert( + std::is_move_constructible_v && std::is_move_constructible_v); + } // https://en.cppreference.com/w/cpp/utility/expected/expected#Version_4 // (implicit) @@ -735,7 +741,7 @@ class expected { storage_.template emplace(std::move(tmp)); } else { static_assert(std::is_nothrow_move_constructible_v); - Current tmp(*std::move(*this)); + Current tmp(std::move(std::get(storage_))); try { storage_.template emplace(std::forward(args)...); } catch (...) { diff --git a/thrift/compiler/whisker/test/expected_test.cc b/thrift/compiler/whisker/test/expected_test.cc index 97390160949..61d3fb40144 100644 --- a/thrift/compiler/whisker/test/expected_test.cc +++ b/thrift/compiler/whisker/test/expected_test.cc @@ -85,6 +85,30 @@ struct move_only { move_only& operator=(move_only&&) noexcept = default; }; +struct non_movable { + non_movable() = default; + non_movable(non_movable&&) = delete; + non_movable& operator=(non_movable&&) = delete; +}; + +struct maythrow_movable { + maythrow_movable(bool should_throw = false) noexcept + : should_throw(should_throw) {} + maythrow_movable(maythrow_movable&&) noexcept(false) { + if (should_throw) { + throw std::runtime_error("maythrow_movable(maythrow_movable&&)"); + } + } + maythrow_movable& operator=(maythrow_movable&&) noexcept(false) { + if (should_throw) { + throw std::runtime_error("maythrow_movable::operator="); + } + return *this; + } + + bool should_throw; +}; + } // namespace TEST(ExpectedTest, construct_default) { @@ -134,6 +158,28 @@ TEST(ExpectedTest, construct_move_only) { EXPECT_TRUE(e2.has_value()); } +TEST(ExpectedTest, construct_value_with_move_only_error) { + expected e1; + expected e2 = std::move(e1); + EXPECT_EQ(std::move(e2).value(), 0); +} + +TEST(ExpectedTest, construct_non_movable) { + expected e1; + EXPECT_TRUE(e1.has_value()); + e1 = unexpected(1); + EXPECT_FALSE(e1.has_value()); + EXPECT_EQ(e1.error(), 1); +} + +TEST(ExpectedTest, construct_value_with_non_movable_error) { + expected e1{unexpect}; + EXPECT_FALSE(e1.has_value()); + e1 = 1; + EXPECT_TRUE(e1.has_value()); + EXPECT_EQ(*e1, 1); +} + TEST(ExpectedTest, construct_error) { expected e = unexpected(1); EXPECT_FALSE(e.has_value()); @@ -215,6 +261,12 @@ TEST(ExpectedTest, emplace_move_only) { EXPECT_TRUE(e.has_value()); } +TEST(ExpectedTest, emplace_non_movable) { + expected e = unexpected(1); + e.emplace(); + EXPECT_TRUE(e.has_value()); +} + TEST(ExpectedTest, emplace_never_empty) { class throw_on_construct { public: @@ -340,6 +392,39 @@ TEST(ExpectedTest, assign_move_only) { EXPECT_TRUE(e1.has_value()); } +TEST(ExpectedTest, assign_move_only_error) { + expected e1 = unexpected(move_only()); + expected e2; + e2 = std::move(e1); + EXPECT_FALSE(e2.has_value()); +} + +TEST(ExpectedTest, assign_maythrow_movable) { + expected e1 = unexpected(1); + expected e2; + e2 = std::move(e1); + EXPECT_EQ(e2.error(), 1); + + expected e3{std::in_place, true}; + expected e4 = unexpected(1); + EXPECT_THROW((e4 = std::move(e3)), std::runtime_error); + EXPECT_FALSE(e4.has_value()); + EXPECT_EQ(e4.error(), 1); +} + +TEST(ExpectedTest, assign_maythrow_movable_error) { + expected e1 = unexpected(1); + expected e2; + e2 = std::move(e1); + EXPECT_FALSE(e2.has_value()); + + expected e3{unexpect, true}; + expected e4 = 1; + EXPECT_THROW((e4 = std::move(e3)), std::runtime_error); + EXPECT_TRUE(e4.has_value()); + EXPECT_EQ(*e4, 1); +} + TEST(ExpectedTest, comparison) { expected e1 = 42; expected e2 = 42;