diff --git a/include/etl/private/variant_variadic.h b/include/etl/private/variant_variadic.h index f3d3dee7..545fd5e8 100644 --- a/include/etl/private/variant_variadic.h +++ b/include/etl/private/variant_variadic.h @@ -1134,8 +1134,12 @@ namespace etl template void do_construct_impl(U&& value, etl::integral_constant) { - // Trivially destructible: assign directly into the variadic_union. - private_variant::variadic_union_get::value>(data) = etl::forward(value); + // Begin the lifetime of the new alternative via placement new. The target + // union member is not alive after do_destroy(), and the alternative may be + // non-assignable (e.g. reference or const members), so assignment would be + // both undefined behaviour and ill-formed for such types. Use etl::addressof + // so the raw storage address is used even for types that overload operator&. + ::new (static_cast(etl::addressof(private_variant::variadic_union_get::value>(data)))) T(etl::forward(value)); } template diff --git a/test/test_variant_variadic.cpp b/test/test_variant_variadic.cpp index 080b0550..5b558e05 100644 --- a/test/test_variant_variadic.cpp +++ b/test/test_variant_variadic.cpp @@ -378,6 +378,48 @@ namespace int b; int sum_of_list; }; + + // Trivially destructible but not assignable (deleted copy and move + // assignment). Constructing an alternative of this type exercises the + // trivially-destructible-suite path, which must begin the alternative's + // lifetime with placement new rather than assigning into the union member. + struct NonAssignable + { + explicit NonAssignable(int value_) + : value(value_) + { + } + + NonAssignable(const NonAssignable&) = default; + NonAssignable(NonAssignable&&) = default; + NonAssignable& operator=(const NonAssignable&) = delete; + NonAssignable& operator=(NonAssignable&&) = delete; + + int value; + }; + + //********************************************* + // Trivially destructible type that overloads operator& (here by deleting it). + // Constructing an alternative of this type must obtain the placement-new + // target with etl::addressof, not the built-in unary operator&: taking the + // address with & would be ill-formed (deleted) or, for an operator& that + // returns something other than the object's address, would construct into the + // wrong storage. + struct NonAddressable + { + explicit NonAddressable(int value_) + : value(value_) + { + } + + NonAddressable(const NonAddressable&) = default; + NonAddressable(NonAddressable&&) = default; + + NonAddressable* operator&() = delete; + const NonAddressable* operator&() const = delete; + + int value; + }; } // namespace // Moved from the top of the file otherwise clang has issues with @@ -484,20 +526,40 @@ struct etl::is_copy_constructible : public etl::false_type { }; +template <> +struct etl::is_copy_constructible : public etl::true_type +{ +}; + template <> struct etl::is_move_constructible : public etl::false_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_copy_constructible : public etl::true_type +{ +}; + template <> struct etl::is_move_constructible : public etl::false_type { +}; + +template <> +struct etl::is_move_constructible : public etl::true_type +{ }; #endif @@ -1056,6 +1118,81 @@ namespace CHECK_EQUAL(D4("1", "2", "3", "4"), etl::get(variant_etl)); } + //************************************************************************* + // Constructing an alternative must begin the object's lifetime with + // placement new, not assign into the (inactive) union member. A trivially + // destructible but non-assignable type would fail to compile (deleted + // assignment) or invoke undefined behaviour if assignment were used. + // (The trivially-destructible-suite union path is taken when the STL or the + // compiler type-trait built-ins are available; otherwise construction goes + // through the buffer path, which is exercised here just the same.) + TEST(test_construct_trivially_destructible_non_assignable_type) + { + using variant_type = etl::variant; + + // Emplace by type. + variant_type variant_by_type; + variant_by_type.emplace(42); + CHECK(etl::holds_alternative(variant_by_type)); + CHECK_EQUAL(42, etl::get(variant_by_type).value); + + // Emplace by index. + variant_type variant_by_index; + variant_by_index.emplace<1>(43); + CHECK(etl::holds_alternative(variant_by_index)); + CHECK_EQUAL(43, etl::get(variant_by_index).value); + + // Assignment from a value (operator=(T&&)). + variant_type variant_by_assignment; + variant_by_assignment = NonAssignable(44); + CHECK(etl::holds_alternative(variant_by_assignment)); + CHECK_EQUAL(44, etl::get(variant_by_assignment).value); + + // Re-emplace over an already-active alternative (do_destroy then + // placement new into the same union member). + variant_by_assignment.emplace(45); + CHECK(etl::holds_alternative(variant_by_assignment)); + CHECK_EQUAL(45, etl::get(variant_by_assignment).value); + } + + //************************************************************************* + // Constructing an alternative must take the raw storage address for + // placement new via etl::addressof, not the built-in unary operator&. + // NonAddressable deletes operator&, so the old `&variadic_union_get(...)` + // form would be ill-formed. Read-back uses by-index etl::get (which returns + // a reference) and the reference returned by emplace, so it never invokes + // operator&. (The trivially-destructible-suite union path is taken when the + // STL or the compiler built-ins are available; otherwise construction goes + // through the buffer path, which is exercised here just the same.) + TEST(test_construct_type_with_overloaded_address_of_operator) + { + using variant_type = etl::variant; + + // Emplace by type (returns a reference, so no operator& on read-back). + variant_type variant_by_type; + NonAddressable& by_type = variant_by_type.emplace(42); + CHECK(etl::holds_alternative(variant_by_type)); + CHECK_EQUAL(42, by_type.value); + CHECK_EQUAL(42, etl::get<1>(variant_by_type).value); + + // Emplace by index. + variant_type variant_by_index; + variant_by_index.emplace<1>(43); + CHECK(etl::holds_alternative(variant_by_index)); + CHECK_EQUAL(43, etl::get<1>(variant_by_index).value); + + // Assignment from a value (operator=(T&&)). + variant_type variant_by_assignment; + variant_by_assignment = NonAddressable(44); + CHECK(etl::holds_alternative(variant_by_assignment)); + CHECK_EQUAL(44, etl::get<1>(variant_by_assignment).value); + + // Re-emplace over an already-active alternative. + variant_by_assignment.emplace<1>(45); + CHECK(etl::holds_alternative(variant_by_assignment)); + CHECK_EQUAL(45, etl::get<1>(variant_by_assignment).value); + } + //************************************************************************* TEST(test_variant_accept_visitor) {