From 18fb69edd4afd05adf958fbb39eabfe61f2360ce Mon Sep 17 00:00:00 2001 From: Wadi <96820481+TWadi@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:35:20 +0200 Subject: [PATCH] Share one dispatch for variant assign and construct (#1620) * Add docs and tutorial for etl::intrusive_avl_tree (#1608) * Add docs and tutorial for etl::intrusive_avl_tree Documents the intrusive AVL tree added in PR #1425: a full API reference page under Containers/Trees, and a tutorial demonstrating ordered access to intrusively-linked objects with unpredictable lifetimes, including move support and destructor-driven automatic removal from the tree. Co-Authored-By: Claude Sonnet 5 * Add complexity shortcode and document its usage Introduces a {{< complexity >}} Hugo shortcode (with light/dark styling for constant/logarithmic/linear/linearithmic/polynomial/exponential classes) for rendering Big-O complexity inline, applies it throughout the intrusive_avl_tree API reference in place of raw "O(...)" text, and documents the Description/Complexity structure and supported classes in DOCUMENTATION.md. Co-Authored-By: Claude Sonnet 5 * Fix issues found by @CodeRabbit * Put complexity notes on their own lines in intrusive_avl_tree docs Move the complexity shortcode from the inline "**Complexity:**" label to its own line under a "**Complexity**" label. Notes that give several alternatives (e.g. amortized vs. worst-case) now show each one on a separate line. Update the guideline in DOCUMENTATION.md to match. * Update complexity examples in DOCUMENTATION.md Add the hard line break after the "**Complexity**" label in the example, and rewrite the list of supported complexity classes in the new two-line format, including an example with several alternatives. * Add logarithmic complexity example to DOCUMENTATION.md --------- Co-authored-by: Claude Sonnet 5 Co-authored-by: John Wellbelove * Updated sponsors pages * Refactor the documentation css to group styles for light and dark themes * Update Codacy badge in README.md Updated Codacy badge link to reflect new project status. * Fix Codacy badge display * refresh codacy integration status * Share one dispatch for variant assign and construct Since #1514, variant copy and move assignment dispatched on the alternative up to three times: assign when the indices match, otherwise destroy and construct. Assigning or constructing the source alternative now shares a single dispatch, which recovers about half of the flash growth reported in #1619 while keeping the std::variant assignment semantics. Add tests for move assignment between alternatives, assignment between alternatives on the trivially destructible path, destruction of the previous alternative, and a throwing copy leaving the variant valueless. --------- Co-authored-by: Sergei Co-authored-by: Claude Sonnet 5 Co-authored-by: John Wellbelove Co-authored-by: John Wellbelove --- include/etl/private/variant_variadic.h | 102 ++++++++-------- test/test_variant_variadic.cpp | 156 +++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 52 deletions(-) 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.