From 4d40fa14fb933164967fa667ebf4a927957dc802 Mon Sep 17 00:00:00 2001 From: songgaoye Date: Fri, 23 Oct 2026 17:32:58 +0800 Subject: [PATCH 1/4] fix: preserve optional value lifetime --- .github/workflows/ci-linux.yml | 7 +++ src/butil/containers/optional.h | 45 ++++++++++++++-- test/optional_unittest.cpp | 94 +++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci-linux.yml b/.github/workflows/ci-linux.yml index 9dcadae328..eff5b4eaba 100644 --- a/.github/workflows/ci-linux.yml +++ b/.github/workflows/ci-linux.yml @@ -387,6 +387,13 @@ jobs: - uses: ./.github/actions/init-ut-make-config with: options: --with-bthread-tracer --with-asan + - name: Test C++14 optional with sanitizers + run: | + clang++-12 -std=c++14 -DBRPC_TEST_OPTIONAL_CXX14 \ + -fsanitize=address,undefined -fno-omit-frame-pointer -I src \ + test/optional_unittest.cpp -lgtest_main -lgtest -pthread \ + -o "$RUNNER_TEMP/optional-cxx14-unittest" + "$RUNNER_TEMP/optional-cxx14-unittest" - name: compile tests run: | cat config.mk diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index e196958496..6fc1f4aab2 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -141,9 +141,20 @@ class optional { optional(nullopt_t) : optional() {} - optional(const optional& rhs) = default; + optional(const optional& rhs) : _engaged(false) { + if (rhs._engaged) { + _storage.Init(*rhs._storage); + _engaged = true; + } + } - optional(optional&& rhs) noexcept = default; + optional(optional&& rhs) noexcept(std::is_nothrow_move_constructible::value) + : _engaged(false) { + if (rhs._engaged) { + _storage.Init(std::move(*rhs._storage)); + _engaged = true; + } + } template ::value && @@ -240,9 +251,35 @@ class optional { return *this; } - optional& operator=(const optional& rhs) = default; + optional& operator=(const optional& rhs) { + if (this != &rhs) { + if (!rhs._engaged) { + reset(); + } else if (_engaged) { + *_storage = *rhs._storage; + } else { + _storage.Init(*rhs._storage); + _engaged = true; + } + } + return *this; + } - optional& operator=(optional&& rhs) = default; + optional& operator=(optional&& rhs) noexcept( + std::is_nothrow_move_constructible::value && + std::is_nothrow_move_assignable::value) { + if (this != &rhs) { + if (!rhs._engaged) { + reset(); + } else if (_engaged) { + *_storage = std::move(*rhs._storage); + } else { + _storage.Init(std::move(*rhs._storage)); + _engaged = true; + } + } + return *this; + } // Value assignment operators template +#include +#include #include "butil/containers/optional.h" +#if defined(BRPC_TEST_OPTIONAL_CXX14) && __cplusplus != 201402L +#error "The C++14 optional test must use the C++14 implementation" +#endif + namespace { butil::optional empty_optional() { return {}; } +struct LifetimeTracker { + static int live; + static int constructed; + static int destroyed; + + LifetimeTracker() { ++live; ++constructed; } + LifetimeTracker(const LifetimeTracker&) { ++live; ++constructed; } + LifetimeTracker(LifetimeTracker&&) { ++live; ++constructed; } + LifetimeTracker& operator=(const LifetimeTracker&) { return *this; } + LifetimeTracker& operator=(LifetimeTracker&&) { return *this; } + ~LifetimeTracker() { --live; ++destroyed; } + + static void Reset() { live = constructed = destroyed = 0; } +}; + +int LifetimeTracker::live = 0; +int LifetimeTracker::constructed = 0; +int LifetimeTracker::destroyed = 0; + TEST(OptionalTest, sanity) { { butil::optional empty; @@ -86,6 +111,15 @@ TEST(OptionalTest, copy) { non_empty = op; ASSERT_TRUE(non_empty); ASSERT_EQ(10, non_empty); + + const std::string value(128, 'x'); + butil::optional string_source(value); + butil::optional string_copied(string_source); + ASSERT_EQ(value, *string_source); + ASSERT_EQ(value, *string_copied); + + string_copied = string_source; + ASSERT_EQ(value, *string_copied); } TEST(OptionalTest, move) { @@ -105,6 +139,66 @@ TEST(OptionalTest, move) { butil::optional empty_move_assign; empty_move_assign = std::move(empty); ASSERT_FALSE(empty_move_assign); + + butil::optional string_source(std::string(128, 'x')); + butil::optional string_moved(std::move(string_source)); + ASSERT_TRUE(string_source); + ASSERT_EQ(std::string(128, 'x'), *string_moved); + + butil::optional string_move_source(std::string(128, 'y')); + string_moved = std::move(string_move_source); + ASSERT_TRUE(string_move_source); + ASSERT_TRUE(string_moved); + ASSERT_EQ(std::string(128, 'y'), *string_moved); +} + +TEST(OptionalTest, nontrivial_lifetime) { + LifetimeTracker::Reset(); + { + butil::optional source(butil::in_place); + butil::optional copied(source); + butil::optional moved(std::move(source)); + butil::optional assigned; + assigned = copied; + assigned = butil::nullopt; + } + ASSERT_EQ(0, LifetimeTracker::live); + ASSERT_EQ(LifetimeTracker::constructed, LifetimeTracker::destroyed); +} + +TEST(OptionalTest, nontrivial_assignment_states) { + const std::string value(128, 'x'); + butil::optional source(value); + butil::optional empty; + butil::optional target; + + target = empty; + ASSERT_FALSE(target); + target = source; + ASSERT_EQ(value, *target); + target = source; + ASSERT_EQ(value, *target); + target = empty; + ASSERT_FALSE(target); + + target = std::move(empty); + ASSERT_FALSE(target); + target = std::move(source); + ASSERT_EQ(value, *target); + source = value; + target = std::move(source); + ASSERT_EQ(value, *target); + target = std::move(empty); + ASSERT_FALSE(target); +} + +TEST(OptionalTest, move_only_value) { + butil::optional> source(std::unique_ptr(new int(42))); + butil::optional> moved(std::move(source)); + ASSERT_EQ(42, **moved); + butil::optional> assigned; + assigned = std::move(moved); + ASSERT_EQ(42, **assigned); } struct Obj {}; From a21ac2dccb0b3ea3aebb01820b03b7ca24bc213c Mon Sep 17 00:00:00 2001 From: songgaoye Date: Tue, 10 Nov 2026 08:51:43 +0800 Subject: [PATCH 2/4] Fix copy and move traits for C++14 optional --- src/butil/containers/optional.h | 221 +++++++++++++++++++++++--------- test/optional_unittest.cpp | 36 ++++++ 2 files changed, 196 insertions(+), 61 deletions(-) diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index 6fc1f4aab2..a21e59b7c4 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -119,10 +119,136 @@ struct is_constructible_convertible_assignable_from_optional is_convertible_from_optional::value || is_assignable_from_optional::value> {}; +template +struct OptionalCopyConstructible {}; + +template <> +struct OptionalCopyConstructible { + OptionalCopyConstructible() = default; + OptionalCopyConstructible(const OptionalCopyConstructible&) = delete; + OptionalCopyConstructible(OptionalCopyConstructible&&) = default; + OptionalCopyConstructible& operator=(const OptionalCopyConstructible&) = default; + OptionalCopyConstructible& operator=(OptionalCopyConstructible&&) = default; +}; + +template +struct OptionalMoveConstructible {}; + +template <> +struct OptionalMoveConstructible { + OptionalMoveConstructible() = default; + OptionalMoveConstructible(const OptionalMoveConstructible&) = default; + OptionalMoveConstructible(OptionalMoveConstructible&&) = delete; + OptionalMoveConstructible& operator=(const OptionalMoveConstructible&) = default; + OptionalMoveConstructible& operator=(OptionalMoveConstructible&&) = default; +}; + +template +struct OptionalCopyAssignable {}; + +template <> +struct OptionalCopyAssignable { + OptionalCopyAssignable() = default; + OptionalCopyAssignable(const OptionalCopyAssignable&) = default; + OptionalCopyAssignable(OptionalCopyAssignable&&) = default; + OptionalCopyAssignable& operator=(const OptionalCopyAssignable&) = delete; + OptionalCopyAssignable& operator=(OptionalCopyAssignable&&) = default; +}; + +template +struct OptionalMoveAssignable {}; + +template <> +struct OptionalMoveAssignable { + OptionalMoveAssignable() = default; + OptionalMoveAssignable(const OptionalMoveAssignable&) = default; + OptionalMoveAssignable(OptionalMoveAssignable&&) = default; + OptionalMoveAssignable& operator=(const OptionalMoveAssignable&) = default; + OptionalMoveAssignable& operator=(OptionalMoveAssignable&&) = delete; +}; + +template +class OptionalBase { +public: + OptionalBase() : _engaged(false) {} + + OptionalBase(const OptionalBase& rhs) : _engaged(false) { + if (rhs._engaged) { + _storage.Init(*rhs._storage); + _engaged = true; + } + } + + OptionalBase(OptionalBase&& rhs) noexcept( + std::is_nothrow_move_constructible::value) : _engaged(false) { + if (rhs._engaged) { + _storage.Init(std::move(*rhs._storage)); + _engaged = true; + } + } + + ~OptionalBase() { reset(); } + + OptionalBase& operator=(const OptionalBase& rhs) { + if (this != &rhs) { + if (rhs._engaged) { + if (_engaged) { + *_storage = *rhs._storage; + } else { + _storage.Init(*rhs._storage); + _engaged = true; + } + } else { + reset(); + } + } + return *this; + } + + OptionalBase& operator=(OptionalBase&& rhs) noexcept( + std::is_nothrow_move_constructible::value && + std::is_nothrow_move_assignable::value) { + if (this != &rhs) { + if (rhs._engaged) { + if (_engaged) { + *_storage = std::move(*rhs._storage); + } else { + _storage.Init(std::move(*rhs._storage)); + _engaged = true; + } + } else { + reset(); + } + } + return *this; + } + +protected: + void reset() { + if (_engaged) { + _storage.Destroy(); + _engaged = false; + } + } + + bool _engaged; + ManualConstructor _storage; +}; + } // namespace internal template -class optional { +class optional : private internal::OptionalBase, + private internal::OptionalCopyConstructible< + std::is_copy_constructible::value>, + private internal::OptionalMoveConstructible< + std::is_move_constructible::value>, + private internal::OptionalCopyAssignable< + std::is_copy_constructible::value && + std::is_copy_assignable::value>, + private internal::OptionalMoveAssignable< + std::is_move_constructible::value && + std::is_move_assignable::value> { public: typedef T value_type; @@ -137,33 +263,24 @@ class optional { static_assert(!is_array::value, "Instantiation of optional with an array type is ill-formed"); - optional() : _engaged(false) {} + optional() = default; optional(nullopt_t) : optional() {} - optional(const optional& rhs) : _engaged(false) { - if (rhs._engaged) { - _storage.Init(*rhs._storage); - _engaged = true; - } - } + optional(const optional& rhs) = default; - optional(optional&& rhs) noexcept(std::is_nothrow_move_constructible::value) - : _engaged(false) { - if (rhs._engaged) { - _storage.Init(std::move(*rhs._storage)); - _engaged = true; - } - } + optional(optional&& rhs) noexcept( + std::is_nothrow_move_constructible::value) = default; template ::value && std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && std::is_convertible::value, bool> = false> - optional(const optional& rhs) : _engaged(rhs.has_value()) { - if (_engaged) { + optional(const optional& rhs) { + if (rhs) { _storage.Init(*rhs); + _engaged = true; } } @@ -172,9 +289,10 @@ class optional { std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && !std::is_convertible::value, bool> = false> - explicit optional(const optional& rhs) : _engaged(rhs.has_value()) { - if (_engaged) { + explicit optional(const optional& rhs) { + if (rhs) { _storage.Init(*rhs); + _engaged = true; } } @@ -183,9 +301,10 @@ class optional { std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && std::is_convertible::value, bool> = false> - optional(optional&& rhs) : _engaged(rhs.has_value()) { - if (_engaged) { + optional(optional&& rhs) { + if (rhs) { _storage.Init(std::move(*rhs)); + _engaged = true; rhs.reset(); } } @@ -195,33 +314,37 @@ class optional { std::is_constructible::value && !internal::is_constructible_convertible_from_optional::value && !std::is_convertible::value, bool> = false> - explicit optional(optional&& rhs) : _engaged(rhs.has_value()) { - if (_engaged) { + explicit optional(optional&& rhs) { + if (rhs) { _storage.Init(std::move(*rhs)); + _engaged = true; rhs.reset(); } } - optional(const T& value) : _engaged(true) { + optional(const T& value) { _storage.Init(value); + _engaged = true; } - optional(T&& value) : _engaged(true) { + optional(T&& value) { _storage.Init(std::move(value)); + _engaged = true; } template ::value, bool> = false> - explicit optional(in_place_t, Args&&... args) : _engaged(true) { + explicit optional(in_place_t, Args&&... args) { _storage.Init(std::forward(args)...); + _engaged = true; } template &, Args&&...>::value, bool> = false> - optional(in_place_t, std::initializer_list il, Args&&... args) - : _engaged(true) { + optional(in_place_t, std::initializer_list il, Args&&... args) { _storage.Init(il, std::forward(args)...); + _engaged = true; } template , typename std::decay::type>::value && std::is_constructible::value && std::is_convertible::value, bool> = false> - optional(U&& v) : _engaged(true) { + optional(U&& v) { _storage.Init(std::forward(v)); + _engaged = true; } template , typename std::decay::type>::value && std::is_constructible::value && !std::is_convertible::value, bool> = false> - explicit optional(U&& v) : _engaged(true) { + explicit optional(U&& v) { _storage.Init(std::forward(v)); + _engaged = true; } - ~optional() { - reset(); - } + ~optional() = default; optional& operator=(nullopt_t) noexcept { reset(); return *this; } - optional& operator=(const optional& rhs) { - if (this != &rhs) { - if (!rhs._engaged) { - reset(); - } else if (_engaged) { - *_storage = *rhs._storage; - } else { - _storage.Init(*rhs._storage); - _engaged = true; - } - } - return *this; - } + optional& operator=(const optional& rhs) = default; optional& operator=(optional&& rhs) noexcept( std::is_nothrow_move_constructible::value && - std::is_nothrow_move_assignable::value) { - if (this != &rhs) { - if (!rhs._engaged) { - reset(); - } else if (_engaged) { - *_storage = std::move(*rhs._storage); - } else { - _storage.Init(std::move(*rhs._storage)); - _engaged = true; - } - } - return *this; - } + std::is_nothrow_move_assignable::value) = default; // Value assignment operators template _storage; + using internal::OptionalBase::_engaged; + using internal::OptionalBase::_storage; }; template diff --git a/test/optional_unittest.cpp b/test/optional_unittest.cpp index bacbd7c19c..50c85fbf64 100644 --- a/test/optional_unittest.cpp +++ b/test/optional_unittest.cpp @@ -201,6 +201,42 @@ TEST(OptionalTest, move_only_value) { ASSERT_EQ(42, **assigned); } +struct CopyConstructOnly { + CopyConstructOnly() = default; + CopyConstructOnly(const CopyConstructOnly&) = default; + CopyConstructOnly(CopyConstructOnly&&) = delete; + CopyConstructOnly& operator=(const CopyConstructOnly&) = delete; + CopyConstructOnly& operator=(CopyConstructOnly&&) = delete; +}; + +struct AssignOnly { + AssignOnly() = default; + AssignOnly(const AssignOnly&) = delete; + AssignOnly(AssignOnly&&) = delete; + AssignOnly& operator=(const AssignOnly&) = default; + AssignOnly& operator=(AssignOnly&&) = default; +}; + +TEST(OptionalTest, copy_move_traits_follow_value_type) { + using MoveOnly = butil::optional>; + static_assert(!std::is_copy_constructible::value, ""); + static_assert(!std::is_copy_assignable::value, ""); + static_assert(std::is_move_constructible::value, ""); + static_assert(std::is_move_assignable::value, ""); + + using CopyOnly = butil::optional; + static_assert(std::is_copy_constructible::value, ""); + static_assert(std::is_move_constructible::value, ""); + static_assert(!std::is_copy_assignable::value, ""); + static_assert(!std::is_move_assignable::value, ""); + + using NoConstruction = butil::optional; + static_assert(!std::is_copy_constructible::value, ""); + static_assert(!std::is_move_constructible::value, ""); + static_assert(!std::is_copy_assignable::value, ""); + static_assert(!std::is_move_assignable::value, ""); +} + struct Obj {}; struct Convert { From f98d8d7749016506d4f82a8f61f0f09df50207db Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sat, 14 Nov 2026 09:58:25 +0800 Subject: [PATCH 3/4] Fix converting optional move semantics --- src/butil/containers/optional.h | 2 -- test/optional_unittest.cpp | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index a21e59b7c4..e10f08c359 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -305,7 +305,6 @@ class optional : private internal::OptionalBase, if (rhs) { _storage.Init(std::move(*rhs)); _engaged = true; - rhs.reset(); } } @@ -318,7 +317,6 @@ class optional : private internal::OptionalBase, if (rhs) { _storage.Init(std::move(*rhs)); _engaged = true; - rhs.reset(); } } diff --git a/test/optional_unittest.cpp b/test/optional_unittest.cpp index 50c85fbf64..bfd9c55aca 100644 --- a/test/optional_unittest.cpp +++ b/test/optional_unittest.cpp @@ -282,10 +282,12 @@ TEST(OptionalTest, convert) { ASSERT_TRUE(opt_copy->default_ctor); ASSERT_FALSE(opt_copy->move_ctor); - butil::optional opt_move(butil::optional{ butil::in_place }); + butil::optional move_source(butil::in_place); + butil::optional opt_move(std::move(move_source)); ASSERT_TRUE(opt_move); ASSERT_TRUE(opt_move->default_ctor); ASSERT_TRUE(opt_move->move_ctor); + ASSERT_TRUE(move_source); } { From d6921e2d8e4a63d54ec8ba63417ca300eb01d4cd Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sat, 14 Nov 2026 12:06:32 +0800 Subject: [PATCH 4/4] Fix optional swap noexcept specification --- src/butil/containers/optional.h | 8 ++++++-- test/optional_unittest.cpp | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/butil/containers/optional.h b/src/butil/containers/optional.h index e10f08c359..4c44a727fa 100644 --- a/src/butil/containers/optional.h +++ b/src/butil/containers/optional.h @@ -496,7 +496,9 @@ class optional : private internal::OptionalBase, std::move(**this) : static_cast(std::forward(v)); } - void swap(optional& rhs) noexcept { + void swap(optional& rhs) noexcept( + std::is_nothrow_move_constructible::value && + std::is_nothrow_move_assignable::value) { if (_engaged && rhs._engaged) { std::swap(**this, *rhs); } else if (_engaged) { @@ -541,7 +543,9 @@ class optional : private internal::OptionalBase, }; template -void swap(optional& a, optional& b) noexcept { a.swap(b); } +void swap(optional& a, optional& b) noexcept(noexcept(a.swap(b))) { + a.swap(b); +} // Creates a non-empty 'optional`. template diff --git a/test/optional_unittest.cpp b/test/optional_unittest.cpp index bfd9c55aca..bb15af3de3 100644 --- a/test/optional_unittest.cpp +++ b/test/optional_unittest.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include "butil/containers/optional.h" #if defined(BRPC_TEST_OPTIONAL_CXX14) && __cplusplus != 201402L @@ -356,6 +357,22 @@ TEST(OptionalTest, swap) { ASSERT_TRUE(noexcept(swap(opt1, opt2))); } +struct ThrowingMove { + ThrowingMove() = default; + ThrowingMove(const ThrowingMove&) = default; + ThrowingMove(ThrowingMove&&) noexcept(false) { } + ThrowingMove& operator=(const ThrowingMove&) = default; + ThrowingMove& operator=(ThrowingMove&&) noexcept(false) { return *this; } +}; + +TEST(OptionalTest, swap_noexcept_follows_value_type) { + using Optional = butil::optional; + static_assert(!noexcept(std::declval().swap( + std::declval())), ""); + static_assert(!noexcept(swap(std::declval(), + std::declval())), ""); +} + TEST(OptionalTest, make_optional) { auto opt_int = butil::make_optional(1); ASSERT_TRUE((std::is_same>::value));