On Fri, 29 Aug 2025, Patrick Palka wrote:
> On Fri, 29 Aug 2025, 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.
> > ---
> > 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.
> >
> >
> > 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;
>
> I believe this makes _Variadic_union instantiation quadratic with
(or rather, more quadratic -- instantiating recursive partial
specializations of the form A<T, Ts...> is always quadratic as we need
to build N argument packs of size 1,...,N to pass as Ts.)
> respect to the number of alternatives. How compile-time/memory
> usage fare for the 87619.cc test with this change? (can use
> -ftime-report to measure that)
>
> > };
> >
> > // _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
> >
> >
