diff --git a/include/etl/private/variant_variadic.h b/include/etl/private/variant_variadic.h index 4161aa31..d0d5674b 100644 --- a/include/etl/private/variant_variadic.h +++ b/include/etl/private/variant_variadic.h @@ -94,8 +94,8 @@ namespace etl static void destroy(char*, size_t) {} static void copy(char*, const char*, size_t) {} static void move(char*, const char*, size_t) {} - static void copy_assign(char*, const char*, size_t) {} - static void move_assign(char*, const char*, size_t) {} + static void copy_assign_or_construct(char*, const char*, size_t, bool) {} + static void move_assign_or_construct(char*, const char*, size_t, bool) {} }; // Recursive case. @@ -148,27 +148,49 @@ namespace etl } } - static void copy_assign(char* dst, const char* src, size_t type_id) + //************************************************************************* + // copy_assign_or_construct / move_assign_or_construct + // + // One dispatch on the source alternative serves both halves of variant + // assignment: the alternative is assigned when the destination already + // holds it, otherwise it is constructed into storage the caller has + // already emptied. Sharing the dispatch keeps variant assignment small. + //************************************************************************* + static void copy_assign_or_construct(char* dst, const char* src, size_t type_id, bool construct) { if (type_id == Index) { - copy_assign_impl(dst, src, etl::integral_constant::value>{}); + if (construct) + { + copy_impl(dst, src, etl::integral_constant::value>{}); + } + else + { + copy_assign_impl(dst, src, etl::integral_constant::value>{}); + } } else { - variant_operations::copy_assign(dst, src, type_id); + variant_operations::copy_assign_or_construct(dst, src, type_id, construct); } } - static void move_assign(char* dst, const char* src, size_t type_id) + static void move_assign_or_construct(char* dst, const char* src, size_t type_id, bool construct) { if (type_id == Index) { - move_assign_impl(dst, src, etl::integral_constant::value>{}); + if (construct) + { + move_impl(dst, src, etl::integral_constant::value>{}); + } + else + { + move_assign_impl(dst, src, etl::integral_constant::value>{}); + } } else { - variant_operations::move_assign(dst, src, type_id); + variant_operations::move_assign_or_construct(dst, src, type_id, construct); } } @@ -521,23 +543,16 @@ namespace etl { if (this != &other) { - if ((type_id != variant_npos) && (type_id == other.type_id)) + if ((type_id != variant_npos) && (type_id != other.type_id)) { - variant_operations<0, TTypes...>::copy_assign(data, other.data, type_id); + variant_operations<0, TTypes...>::destroy(data, type_id); + type_id = variant_npos; } - else - { - if (type_id != variant_npos) - { - variant_operations<0, TTypes...>::destroy(data, type_id); - type_id = variant_npos; - } - if (other.type_id != variant_npos) - { - variant_operations<0, TTypes...>::copy(data, other.data, other.type_id); - type_id = other.type_id; - } + if (other.type_id != variant_npos) + { + variant_operations<0, TTypes...>::copy_assign_or_construct(data, other.data, other.type_id, type_id == variant_npos); + type_id = other.type_id; } } @@ -549,23 +564,16 @@ namespace etl { if (this != &other) { - if ((type_id != variant_npos) && (type_id == other.type_id)) + if ((type_id != variant_npos) && (type_id != other.type_id)) { - variant_operations<0, TTypes...>::move_assign(data, other.data, type_id); + variant_operations<0, TTypes...>::destroy(data, type_id); + type_id = variant_npos; } - else - { - if (type_id != variant_npos) - { - variant_operations<0, TTypes...>::destroy(data, type_id); - type_id = variant_npos; - } - if (other.type_id != variant_npos) - { - variant_operations<0, TTypes...>::move(data, other.data, other.type_id); - type_id = other.type_id; - } + if (other.type_id != variant_npos) + { + variant_operations<0, TTypes...>::move_assign_or_construct(data, other.data, other.type_id, type_id == variant_npos); + type_id = other.type_id; } } @@ -687,13 +695,7 @@ namespace etl void copy_assign_from(const variant_trivially_destructible_base& other) { - if ((type_id != variant_npos) && (type_id == other.type_id)) - { - variant_operations<0, TTypes...>::copy_assign(reinterpret_cast(&data), reinterpret_cast(&other.data), type_id); - return; - } - - if (type_id != variant_npos) + if ((type_id != variant_npos) && (type_id != other.type_id)) { variant_operations<0, TTypes...>::destroy(reinterpret_cast(&data), type_id); type_id = variant_npos; @@ -701,20 +703,15 @@ namespace etl if (other.type_id != variant_npos) { - variant_operations<0, TTypes...>::copy(reinterpret_cast(&data), reinterpret_cast(&other.data), other.type_id); + variant_operations<0, TTypes...>::copy_assign_or_construct(reinterpret_cast(&data), reinterpret_cast(&other.data), + other.type_id, type_id == variant_npos); type_id = other.type_id; } } void move_assign_from(variant_trivially_destructible_base& other) { - if ((type_id != variant_npos) && (type_id == other.type_id)) - { - variant_operations<0, TTypes...>::move_assign(reinterpret_cast(&data), reinterpret_cast(&other.data), type_id); - return; - } - - if (type_id != variant_npos) + if ((type_id != variant_npos) && (type_id != other.type_id)) { variant_operations<0, TTypes...>::destroy(reinterpret_cast(&data), type_id); type_id = variant_npos; @@ -722,7 +719,8 @@ namespace etl if (other.type_id != variant_npos) { - variant_operations<0, TTypes...>::move(reinterpret_cast(&data), reinterpret_cast(&other.data), other.type_id); + variant_operations<0, TTypes...>::move_assign_or_construct(reinterpret_cast(&data), reinterpret_cast(&other.data), + other.type_id, type_id == variant_npos); type_id = other.type_id; } } diff --git a/test/test_variant_variadic.cpp b/test/test_variant_variadic.cpp index 8e81a606..c8ce26b7 100644 --- a/test/test_variant_variadic.cpp +++ b/test/test_variant_variadic.cpp @@ -543,6 +543,30 @@ namespace bool copy_assigned; bool move_assigned; }; + + //********************************************* + // Counts destructor calls, to verify that variant assignment destroys the + // previous alternative only when the alternative changes. + struct DestructorCounter + { + DestructorCounter() {} + + DestructorCounter(const DestructorCounter&) {} + + ~DestructorCounter() + { + ++destructor_calls; + } + + DestructorCounter& operator=(const DestructorCounter&) + { + return *this; + } + + static int destructor_calls; + }; + + int DestructorCounter::destructor_calls = 0; } // namespace // Moved from the top of the file otherwise clang has issues with @@ -1160,6 +1184,73 @@ namespace CHECK_EQUAL(etl::variant_npos, v.index()); } } + + //************************************************************************* + // Assigning a different alternative whose copy constructor throws must + // leave the variant valueless-after-exception. + struct ThrowOnCopy + { + struct exception + { + }; + + ThrowOnCopy() {} + + ThrowOnCopy(const ThrowOnCopy&) + { + throw exception(); + } + + ThrowOnCopy& operator=(const ThrowOnCopy&) + { + return *this; + } + }; + + TEST(test_copy_assign_throwing_different_type_is_valueless_by_exception) + { + // Trivially destructible suite (variadic_union storage). + { + etl::variant v1(etl::in_place_type_t{}, 42); + etl::variant v2; + v2.emplace(); + + bool threw = false; + try + { + v1 = v2; + } + catch (const ThrowOnCopy::exception&) + { + threw = true; + } + + CHECK(threw); + CHECK(v1.valueless_by_exception()); + CHECK_EQUAL(etl::variant_npos, v1.index()); + } + + // Non-trivially destructible suite (uninitialized_buffer storage). + { + etl::variant v1(etl::in_place_type_t{}, "Some Text"); + etl::variant v2; + v2.emplace(); + + bool threw = false; + try + { + v1 = v2; + } + catch (const ThrowOnCopy::exception&) + { + threw = true; + } + + CHECK(threw); + CHECK(v1.valueless_by_exception()); + CHECK_EQUAL(etl::variant_npos, v1.index()); + } + } #endif //************************************************************************* @@ -1324,6 +1415,71 @@ namespace CHECK(etl::get(variant_1).move_assigned); } + //************************************************************************* + TEST(test_move_assign_different_type_reconstructs) + { + typedef etl::variant tracker_variant; + + tracker_variant variant_1(etl::in_place_type_t{}, 5); + tracker_variant variant_2(etl::in_place_type_t{}, 2); + + variant_1 = etl::move(variant_2); + + CHECK_EQUAL(1U, variant_1.index()); + CHECK_EQUAL(2, etl::get(variant_1).value); + CHECK(etl::get(variant_1).constructed); + CHECK(!etl::get(variant_1).copy_assigned); + CHECK(!etl::get(variant_1).move_assigned); + } + + //************************************************************************* + TEST(test_assign_different_type_reconstructs_trivial) + { + typedef etl::variant tracker_variant; + + tracker_variant variant_1(etl::in_place_type_t{}, 5); + tracker_variant variant_2(etl::in_place_type_t{}, 2); + + variant_1 = variant_2; + + CHECK_EQUAL(1U, variant_1.index()); + CHECK_EQUAL(2, etl::get(variant_1).value); + CHECK(!etl::get(variant_1).copy_assigned); + CHECK(!etl::get(variant_1).move_assigned); + + tracker_variant variant_3(etl::in_place_type_t{}, 6); + tracker_variant variant_4(etl::in_place_type_t{}, 4); + + variant_3 = etl::move(variant_4); + + CHECK_EQUAL(1U, variant_3.index()); + CHECK_EQUAL(4, etl::get(variant_3).value); + CHECK(!etl::get(variant_3).copy_assigned); + CHECK(!etl::get(variant_3).move_assigned); + } + + //************************************************************************* + TEST(test_assign_destroys_previous_alternative_only_when_it_changes) + { + typedef etl::variant counter_variant; + + counter_variant variant_1(etl::in_place_type_t{}); + counter_variant variant_2(etl::in_place_type_t{}); + counter_variant variant_3(etl::in_place_type_t{}, 3); + + DestructorCounter::destructor_calls = 0; + variant_1 = variant_2; + + CHECK_EQUAL(0, DestructorCounter::destructor_calls); + CHECK_EQUAL(0U, variant_1.index()); + + variant_1 = variant_3; + + CHECK_EQUAL(1, DestructorCounter::destructor_calls); + CHECK_EQUAL(1U, variant_1.index()); + CHECK_EQUAL(3, etl::get(variant_1)); + } + //************************************************************************* // Issue #1567: the variant propagates trivial copyability from its // alternatives, as required by P0602R4.