From 5cd2f28cd54e4019f7be126bcf7394a6a8318eb9 Mon Sep 17 00:00:00 2001 From: Drew Rife Date: Tue, 7 Jul 2026 03:14:48 -0400 Subject: [PATCH] Refactor variant::emplace to perfect forward args instead of copy/move construct (#1494) * refactor: emplace logic to use do_emplace for in-place construction with forwarded arguments #1493 * Add test for variant_variadic --------- Co-authored-by: Roland Reichwein --- include/etl/private/variant_variadic.h | 34 +++++- test/test_variant_variadic.cpp | 160 +++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 4 deletions(-) diff --git a/include/etl/private/variant_variadic.h b/include/etl/private/variant_variadic.h index 3b621582..f3d3dee7 100644 --- a/include/etl/private/variant_variadic.h +++ b/include/etl/private/variant_variadic.h @@ -715,7 +715,7 @@ namespace etl using type = etl::remove_cvref_t; do_destroy(); - do_construct(type(etl::forward(args)...)); + do_emplace(etl::forward(args)...); type_id = index_of_type::value; @@ -735,7 +735,7 @@ namespace etl using type = etl::remove_cvref_t; do_destroy(); - do_construct(type(il, etl::forward(args)...)); + do_emplace(il, etl::forward(args)...); type_id = index_of_type::value; @@ -755,7 +755,7 @@ namespace etl using type = type_from_index; do_destroy(); - do_construct(type(etl::forward(args)...)); + do_emplace(etl::forward(args)...); type_id = Index; @@ -775,7 +775,7 @@ namespace etl using type = type_from_index; do_destroy(); - do_construct(type(il, etl::forward(args)...)); + do_emplace(il, etl::forward(args)...); type_id = Index; @@ -1096,6 +1096,32 @@ namespace etl private_variant::variant_operations<0, TTypes...>::destroy(data, type_id); } + //*************************************************************************** + /// Emplace-construct the alternative in place from forwarded args. + /// No temporary, so no move/copy is required (works for move-hostile types). + //*************************************************************************** + template + void do_emplace(TArgs&&... args) + { + do_emplace_impl(etl::integral_constant{}, etl::forward(args)...); + } + + // Trivially destructible suite: storage is a variadic_union. The old member is + // trivially destructible, so placement-new of the new member is well-defined at + // runtime (emplace is not constexpr, so placement-new is permitted here). + template + void do_emplace_impl(etl::integral_constant, TArgs&&... args) + { + ::new (static_cast(etl::addressof(private_variant::variadic_union_get::value>(data)))) T(etl::forward(args)...); + } + + // Non-trivially destructible suite: storage is an uninitialized_buffer. + template + void do_emplace_impl(etl::integral_constant, TArgs&&... args) + { + ::new (static_cast(data)) T(etl::forward(args)...); + } + //*************************************************************************** /// Construct a value in the union or buffer storage. //*************************************************************************** diff --git a/test/test_variant_variadic.cpp b/test/test_variant_variadic.cpp index 01ff7ded..b9f7a04b 100644 --- a/test/test_variant_variadic.cpp +++ b/test/test_variant_variadic.cpp @@ -320,6 +320,64 @@ namespace bool moved_to; bool copied_to; }; + + //********************************************* + // Trivially destructible, but non-copyable and non-movable. + // Exercises the trivially-destructible (variadic_union) emplace path. + struct TrivialNonMovable + { + TrivialNonMovable(int a_, int b_) + : a(a_) + , b(b_) + { + } + + TrivialNonMovable(const TrivialNonMovable&) = delete; + TrivialNonMovable(TrivialNonMovable&&) = delete; + TrivialNonMovable& operator=(const TrivialNonMovable&) = delete; + TrivialNonMovable& operator=(TrivialNonMovable&&) = delete; + + int a; + int b; + }; + + //********************************************* + // Non-trivially destructible, non-copyable and non-movable. + // Exercises the non-trivially-destructible (uninitialized_buffer) emplace path, + // including the initializer_list overload. + struct NonTrivialNonMovable + { + NonTrivialNonMovable(int a_, int b_) + : a(a_) + , b(b_) + , sum_of_list(0) + { + } + + NonTrivialNonMovable(std::initializer_list il, int b_) + : a(0) + , b(b_) + , sum_of_list(0) + { + for (int value : il) + { + sum_of_list += value; + } + } + + ~NonTrivialNonMovable() // Makes it non-trivially destructible. + { + } + + NonTrivialNonMovable(const NonTrivialNonMovable&) = delete; + NonTrivialNonMovable(NonTrivialNonMovable&&) = delete; + NonTrivialNonMovable& operator=(const NonTrivialNonMovable&) = delete; + NonTrivialNonMovable& operator=(NonTrivialNonMovable&&) = delete; + + int a; + int b; + int sum_of_list; + }; } // namespace // Moved from the top of the file otherwise clang has issues with @@ -418,6 +476,28 @@ struct etl::is_copy_constructible : public etl::true_type template <> struct etl::is_move_constructible : public etl::true_type { +}; + +//************************* +template <> +struct etl::is_copy_constructible : public etl::false_type +{ +}; + +template <> +struct etl::is_move_constructible : public etl::false_type +{ +}; + +//************************* +template <> +struct etl::is_copy_constructible : public etl::false_type +{ +}; + +template <> +struct etl::is_move_constructible : public etl::false_type +{ }; #endif @@ -732,6 +812,86 @@ namespace CHECK_EQUAL(text, etl::get(variant_text_etl)); } + //************************************************************************* + // emplace must construct the alternative in place from the forwarded + // arguments, without requiring the alternative to be copyable or movable + // (see issue #1493). + TEST(test_emplace_non_movable_type) + { + // Trivially destructible suite (variadic_union storage). + { + etl::variant v; + + TrivialNonMovable& r1 = v.emplace(3, 4); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(3, r1.a); + CHECK_EQUAL(4, r1.b); + CHECK_EQUAL(&r1, &etl::get(v)); + + // Emplace by index over the existing alternative. + TrivialNonMovable& r2 = v.emplace<1>(5, 6); + CHECK_EQUAL(5, r2.a); + CHECK_EQUAL(6, r2.b); + + // Switch to the trivial alternative and back again. + v.emplace(42); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(42, etl::get(v)); + + TrivialNonMovable& r3 = v.emplace(7, 8); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(7, r3.a); + CHECK_EQUAL(8, r3.b); + } + + // Non-trivially destructible suite (uninitialized_buffer storage). + { + etl::variant v; + + NonTrivialNonMovable& r1 = v.emplace(1, 2); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(1, r1.a); + CHECK_EQUAL(2, r1.b); + CHECK_EQUAL(&r1, &etl::get(v)); + + // Emplace by index over the existing alternative. + NonTrivialNonMovable& r2 = v.emplace<1>(9, 10); + CHECK_EQUAL(9, r2.a); + CHECK_EQUAL(10, r2.b); + + // Switch to the std::string alternative and back again. + v.emplace("Some Text"); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(std::string("Some Text"), etl::get(v)); + + NonTrivialNonMovable& r3 = v.emplace(11, 12); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(11, r3.a); + CHECK_EQUAL(12, r3.b); + } + } + + #if ETL_HAS_INITIALIZER_LIST + //************************************************************************* + // The initializer_list emplace overloads must also construct in place, + // without requiring the alternative to be copyable or movable (#1493). + TEST(test_emplace_non_movable_type_with_initializer_list) + { + etl::variant v; + + // By type. + NonTrivialNonMovable& r1 = v.emplace({10, 20, 30}, 99); + CHECK(etl::holds_alternative(v)); + CHECK_EQUAL(60, r1.sum_of_list); + CHECK_EQUAL(99, r1.b); + + // By index. + NonTrivialNonMovable& r2 = v.emplace<1>({1, 2, 3, 4}, 7); + CHECK_EQUAL(10, r2.sum_of_list); + CHECK_EQUAL(7, r2.b); + } + #endif + //************************************************************************* TEST(test_copy_constructor) {