On Wed, Mar 4, 2026 at 4:11 PM Jonathan Wakely <[email protected]> wrote:
> On Wed, 25 Feb 2026 at 14:10 +0100, Tomasz Kamiński wrote: > >The _Arg_value::_M_set method, initialized the union member, by > >assining to reference to that member produced by _M_get(*this). > >However, per language rules, such assigment has undefined behavior, > >if alternative was not already active. > > > So doing 'u.mem = val' changes the active member, but returning a > reference to u.mem from the function and then assigning to that > reference is undefined, because we can't do 'return u.mem' if it's not > active? Interesting. > Yes, the rules are syntax based. From what I recall, allowing any reference would lead into an arbitrary function accepting int* p, being able to switch union lifetime by assigning to *p. I think the exact need here is for the compiler to be able to see that this is an assignment for a union member., > > Ah yes, [class.union.general] p5. Could you add that standard > reference to the commit message, maybe "per the language rules in > [class.union.general], such an assignment has ..." > Will do, however I do not think that p5 is relevant here. Assigning to object not withing lifetime is UB, and p5 does not make it non UB. > > >To address above, we modify _M_set to use placement new for the class > >types, and invoke _S_access with two arguments for all other types. > >The _S_access (rename of _S_get) is modified to assign the value of > >the second parameter (if provided) to the union memnber. Such direct > >assigments are treated specially in the language, and will start > I will add the reference to [class.union.general] p5, here, as that were we get special treatment. > >lifetime of union alternative of implicit-lifetime type. > > > >libstdc++-v3/ChangeLog: > > > > * include/std/format (_Arg_value::_M_get): Rename to... > > (_Arg_value::_M_access): Modified to accept optional > > second parameter that is assigned to value. > > (_Arg_value::_M_get): Handle rename. > > (_Arg_value::_M_set): Use construct_at for basic_string_view, > > and two-argument _S_access for other types. > > > >Signed-off-by: Tomasz Kamiński <[email protected]> > >Signed-off-by: Ivan Lazaric <[email protected]> > >Co-authored-by: Ivan Lazaric <[email protected]> > >--- > >This was detected by constant evaluator, and while it seem to not > >be harmfull at runtime, we will need to address it anyway. > > > >Testing on x86_64-linux. OK for trunk when all test passes? > > > > libstdc++-v3/include/std/format | 61 ++++++++++++++++++--------------- > > 1 file changed, 33 insertions(+), 28 deletions(-) > > > >diff --git a/libstdc++-v3/include/std/format > b/libstdc++-v3/include/std/format > >index 245ea964675..4f0b0f377c6 100644 > >--- a/libstdc++-v3/include/std/format > >+++ b/libstdc++-v3/include/std/format > >@@ -4234,70 +4234,73 @@ namespace __format > > { _S_get<_Tp>() = __val; } > > #endif > > > >- template<typename _Tp, typename _Self> > >+ // Returns reference to the _Arg_value member with the type _Tp. > >+ // Value of second argument (if provided), is assigned to that > member. > >+ template<typename _Tp, typename _Self, typename... _Value> > > [[__gnu__::__always_inline__]] > > static auto& > >- _S_get(_Self& __u) noexcept > >+ _S_access(_Self& __u, _Value... __value) noexcept > > { > >+ static_assert(sizeof...(_Value) <= 1); > > if constexpr (is_same_v<_Tp, bool>) > >- return __u._M_bool; > >+ return (__u._M_bool = ... = __value); > > Oh that's sneaky. Clever, but sneaky. > Took the idea from Ivan patch. > > > else if constexpr (is_same_v<_Tp, _CharT>) > >- return __u._M_c; > >+ return (__u._M_c = ... = __value); > > else if constexpr (is_same_v<_Tp, int>) > >- return __u._M_i; > >+ return (__u._M_i = ... = __value); > > else if constexpr (is_same_v<_Tp, unsigned>) > >- return __u._M_u; > >+ return (__u._M_u = ... = __value); > > else if constexpr (is_same_v<_Tp, long long>) > >- return __u._M_ll; > >+ return (__u._M_ll = ... = __value); > > else if constexpr (is_same_v<_Tp, unsigned long long>) > >- return __u._M_ull; > >+ return (__u._M_ull = ... = __value); > > else if constexpr (is_same_v<_Tp, float>) > >- return __u._M_flt; > >+ return (__u._M_flt = ... = __value); > > else if constexpr (is_same_v<_Tp, double>) > >- return __u._M_dbl; > >+ return (__u._M_dbl = ... = __value); > > #ifndef _GLIBCXX_LONG_DOUBLE_ALT128_COMPAT > > else if constexpr (is_same_v<_Tp, long double>) > >- return __u._M_ldbl; > >+ return (__u._M_ldbl = ... = __value); > > #else > > else if constexpr (is_same_v<_Tp, __ibm128>) > >- return __u._M_ibm128; > >+ return (__u._M_ibm128 = ... = __value); > > else if constexpr (is_same_v<_Tp, __ieee128>) > >- return __u._M_ieee128; > >+ return (__u._M_ieee128 = ... = __value); > > #endif > > #ifdef __SIZEOF_FLOAT128__ > > else if constexpr (is_same_v<_Tp, __float128>) > >- return __u._M_float128; > >+ return (__u._M_float128 = ... = __value); > > #endif > > else if constexpr (is_same_v<_Tp, const _CharT*>) > >- return __u._M_str; > >+ return (__u._M_str = ... = __value); > > else if constexpr (is_same_v<_Tp, basic_string_view<_CharT>>) > >- return __u._M_sv; > >+ return (__u._M_sv = ... = __value); > > else if constexpr (is_same_v<_Tp, const void*>) > >- return __u._M_ptr; > >+ return (__u._M_ptr = ... = __value); > > #ifdef __SIZEOF_INT128__ > > else if constexpr (is_same_v<_Tp, __int128>) > >- return __u._M_i128; > >+ return (__u._M_i128 = ... = __value); > > else if constexpr (is_same_v<_Tp, unsigned __int128>) > >- return __u._M_u128; > >+ return (__u._M_u128 = ... = __value); > > #endif > > #ifdef __BFLT16_DIG__ > > else if constexpr (is_same_v<_Tp, __bflt16_t>) > >- return __u._M_bf16; > >+ return (__u._M_bf16 = ... = __value); > > #endif > > #ifdef __FLT16_DIG__ > > else if constexpr (is_same_v<_Tp, _Float16>) > >- return __u._M_f16; > >+ return (__u._M_f16 = ... = __value); > > #endif > > #ifdef __FLT32_DIG__ > > else if constexpr (is_same_v<_Tp, _Float32>) > >- return __u._M_f32; > >+ return (__u._M_f32 = ... = __value); > > #endif > > #ifdef __FLT64_DIG__ > > else if constexpr (is_same_v<_Tp, _Float64>) > >- return __u._M_f64; > >+ return (__u._M_f64 = ... = __value); > > #endif > > else if constexpr (is_same_v<_Tp, _Handle<_Context>>) > >- return __u._M_handle; > >+ return (__u._M_handle = ... = __value); > > // Otherwise, ill-formed. > > } > > > >@@ -4305,23 +4308,25 @@ namespace __format > > [[__gnu__::__always_inline__]] > > auto& > > _M_get() noexcept > >- { return _S_get<_Tp>(*this); } > >+ { return _S_access<_Tp>(*this); } > > > > template<typename _Tp> > > [[__gnu__::__always_inline__]] > > const auto& > > _M_get() const noexcept > >- { return _S_get<_Tp>(*this); } > >+ { return _S_access<_Tp>(*this); } > > I think I wanted to use 'deducing this' for _M_get, but it wasn't > supported yet when I wrote this code. We would not need to have > _S_access and two _M_get overloads if we just replaced _S_access with > an explicit object member function, _M_access. > Two points: * Deducing this is C++23 feature, and that code targets C++20. This is not critical blocker as we can use that as an extension. But I do not think we have real need to use it (in contrast to example bind). * I preffer _S_access to be separate from the "user"-facing interface, due "cleaver sneaky" tricks inside. > > We can do that for GCC 17. > > > > > template<typename _Tp> > > [[__gnu__::__always_inline__]] > > void > > _M_set(_Tp __v) noexcept > > { > > I think it would be good to add a comment here, something like this > (is "implicit lifetime type" the relevant rule here?): > No, no two places around starting lifetime use the same rules, here the default constructor needs to be trivial ( https://eel.is/c++draft/class.union#general-5.1). and not-deleted. But will add comments. > > // Initialize the non-implicit lifetime types explicitly: > > >- if constexpr (is_same_v<_Tp, _Handle<_Context>>) > >+ if constexpr (is_same_v<_Tp, basic_string_view<_CharT>>) > >+ std::construct_at(&_M_sv, __v); > >+ else if constexpr (is_same_v<_Tp, _Handle<_Context>>) > > std::construct_at(&_M_handle, __v); > > else > > And here: > > else // implicit lifetime type > > >- _S_get<_Tp>(*this) = __v; > >+ _S_access<_Tp>(*this, __v); > > } > > }; > > > >-- > >2.53.0 > > > > > >
