[RFC] libstdc++: Refactor _Variadic_union so _Unitialized<T, false> is not needed.

[Jonathan Wakely]

On Sat, 06 Dec 2025 at 12:30 +0000, Jonathan Wakely wrote:
>On Fri, 29 Aug 2025 at 13:33 +0200, Tomasz Kamiński wrote:
>>The changes the _Variadic_union implementation, in a way that the
>>_Unitialized<T, false> partial specialization for non-trivial types is not
>>necessary.
>>
>>This is simply done by separating the specialization for __trivially_destructible
>>being true and false, and for the later definining an empty destructor (similary
>>as it was done using concepts).
>>
>>We also reduce the number of specialization of _Varidic_union, so specialization
>>(int, int) is reused by (string, int, int) and (int,int). This is done by computing
>>initializating __trivially_destructible with conjuction of is_trivially_destructible_v
>>for remaining components. This is only necessary for non-trivial (false) specialization,
>>as if both _First and _Rest... are trivally destructible, then _Rest must also be.
>>
>>This also add test for PR112591.
>
>OK for trunk.
>
>>---
>>As in title, this is refernce for comments, espcially if we want to land this change,
>>or part of it to the trunk regardless of other changes. I think the test is valuable for sure,
>>but the separate specialization, and removal of _Variadic_union<false, int, int>
>>specialization seem usefull.
>>
>>Locally I have removed the _Unitialized<T, false> specialization, and the objects
>>no longer alias in C++17 mode (112591.cc works the same in C++20/17), and all
>>test have passed locally, so this seem correct. We could even remove the _Unitialized
>>altogether and just store _First direclty. This is however ABI break, as it does
>>affect layout of classes similar to one in the example.
>
>
>We can't change the ABI for C++17 now.
>
>But we also need the ABI for C++17 and C++20 to match ... which
>suggests that we need to change C++20 to have the same bad properties
>as C++17 :-(

Or maybe we just fix it and rely on the fact that the C++17 behaviour
is not helpful to anybody, and is probably quite rare (you have to
have a std::variant member containing the same type as an empty base
class) and that the only people affected by that probably don't want
that behaviour.

>
>>libstdc++-v3/include/std/variant              | 34 ++++++++++++++-----
>>.../testsuite/20_util/variant/112591.cc       | 32 +++++++++++++++++
>>2 files changed, 57 insertions(+), 9 deletions(-)
>>create mode 100644 libstdc++-v3/testsuite/20_util/variant/112591.cc
>>
>>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
>>index 2f44f970028..f2f55837461 100644
>>--- a/libstdc++-v3/include/std/variant
>>+++ b/libstdc++-v3/include/std/variant
>>@@ -393,8 +393,29 @@ namespace __variant
>>	_Variadic_union(in_place_index_t<_Np>, _Args&&...) = delete;
>>    };
>>
>>-  template<bool __trivially_destructible, typename _First, typename... _Rest>
>>-    union _Variadic_union<__trivially_destructible, _First, _Rest...>
>>+  template<typename _First, typename... _Rest>
>>+    union _Variadic_union<true, _First, _Rest...>
>>+    {
>>+      constexpr _Variadic_union() : _M_rest() { }
>>+
>>+      template<typename... _Args>
>>+	constexpr
>>+	_Variadic_union(in_place_index_t<0>, _Args&&... __args)
>>+	: _M_first(in_place_index<0>, std::forward<_Args>(__args)...)
>>+	{ }
>>+
>>+      template<size_t _Np, typename... _Args>
>>+	constexpr
>>+	_Variadic_union(in_place_index_t<_Np>, _Args&&... __args)
>>+	: _M_rest(in_place_index<_Np-1>, std::forward<_Args>(__args)...)
>>+	{ }
>>+
>>+      _Uninitialized<_First> _M_first;
>>+      _Variadic_union<true, _Rest...> _M_rest;
>>+    };
>>+
>>+  template<typename _First, typename... _Rest>
>>+    union _Variadic_union<false, _First, _Rest...>
>>    {
>>      constexpr _Variadic_union() : _M_rest() { }
>>
>>@@ -410,24 +431,19 @@ namespace __variant
>>	: _M_rest(in_place_index<_Np-1>, std::forward<_Args>(__args)...)
>>	{ }
>>
>>-#if __cpp_lib_variant >= 202106L
>>      _Variadic_union(const _Variadic_union&) = default;
>>      _Variadic_union(_Variadic_union&&) = default;
>>      _Variadic_union& operator=(const _Variadic_union&) = default;
>>      _Variadic_union& operator=(_Variadic_union&&) = default;
>>
>>-      ~_Variadic_union() = default;
>>-
>>      // If any alternative type is not trivially destructible then we need a
>>      // user-provided destructor that does nothing. The active alternative
>>      // will be destroyed by _Variant_storage::_M_reset() instead of here.
>>-      constexpr ~_Variadic_union()
>>-	requires (!__trivially_destructible)
>>+      _GLIBCXX20_CONSTEXPR ~_Variadic_union()
>>      { }
>>-#endif
>>
>>      _Uninitialized<_First> _M_first;
>>-      _Variadic_union<__trivially_destructible, _Rest...> _M_rest;
>>+      _Variadic_union<(is_trivially_destructible_v<_Rest> && ...), _Rest...> _M_rest;
>>    };
>>
>>  // _Never_valueless_alt is true for variant alternatives that can
>>diff --git a/libstdc++-v3/testsuite/20_util/variant/112591.cc b/libstdc++-v3/testsuite/20_util/variant/112591.cc
>>new file mode 100644
>>index 00000000000..b1b07c4f46e
>>--- /dev/null
>>+++ b/libstdc++-v3/testsuite/20_util/variant/112591.cc
>>@@ -0,0 +1,32 @@
>>+// { dg-do run { target c++17 } }
>>+
>>+#include <variant>
>>+#include <testsuite_hooks.h>
>>+
>>+struct NonEmpty { int x; };
>>+struct TrivialEmpty {};
>>+struct NonTrivialEmpty { ~NonTrivialEmpty() {} };
>>+
>>+template<typename T>
>>+struct Compose : T
>>+{
>>+  std::variant<T, int> v;
>>+};
>>+
>>+template<typename T>
>>+bool testAlias()
>>+{
>>+  Compose<T> c;
>>+  return static_cast<T*>(&c) == &std::get<T>(c.v);
>>+}
>>+
>>+int main()
>>+{
>>+  VERIFY( !testAlias<NonEmpty>() );
>>+  VERIFY( !testAlias<TrivialEmpty>() );
>>+#if __cplusplus >= 202002L
>>+  VERIFY( !testAlias<NonTrivialEmpty>() );
>>+#else
>>+  VERIFY( testAlias<NonTrivialEmpty>() );
>>+#endif
>>+}
>>-- 
>>2.50.1
>>
>>