| Message ID | 20240821085505.351397-1-jwakely@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] libstdc++: Fix std::variant to reject array types [PR116381] | expand |
On Wed, 21 Aug 2024 at 09:55, Jonathan Wakely wrote: > > Tested x86_64-linux. Pushed to trunk. > > Probably worth backporting too. It could potentially cause new errors > for people using arrays in std::variant, but that's forbidden by the > standard. Notably, both libc++ and MSVC STL reject array types in std::variant. Only libstdc++ had the bug that allowed them.
On Wed, Aug 21, 2024 at 1:56 AM Jonathan Wakely <jwakely@redhat.com> wrote: > > Tested x86_64-linux. Pushed to trunk. > > Probably worth backporting too. It could potentially cause new errors > for people using arrays in std::variant, but that's forbidden by the > standard. It might be worth mentioning in porting_to guide just in case. You never know since we have gotten bug reports about broken code that was also rejected by clang/MSVC due to a change in GCC. Thanks, Andrew > > -- >8 -- > > libstdc++-v3/ChangeLog: > > PR libstdc++/116381 > * include/std/variant (variant): Fix conditions for > static_assert to match the spec. > * testsuite/20_util/variant/types_neg.cc: New test. > --- > libstdc++-v3/include/std/variant | 6 ++---- > .../testsuite/20_util/variant/types_neg.cc | 17 +++++++++++++++++ > 2 files changed, 19 insertions(+), 4 deletions(-) > create mode 100644 libstdc++-v3/testsuite/20_util/variant/types_neg.cc > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > index 12108d07f0b..5fb7770d889 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -1457,10 +1457,8 @@ namespace __detail::__variant > > static_assert(sizeof...(_Types) > 0, > "variant must have at least one alternative"); > - static_assert(!(std::is_reference_v<_Types> || ...), > - "variant must have no reference alternative"); > - static_assert(!(std::is_void_v<_Types> || ...), > - "variant must have no void alternative"); > + static_assert(((std::is_object_v<_Types> && !is_array_v<_Types>) && ...), > + "variant alternatives must be non-array object types"); > > using _Base = __detail::__variant::_Variant_base<_Types...>; > > diff --git a/libstdc++-v3/testsuite/20_util/variant/types_neg.cc b/libstdc++-v3/testsuite/20_util/variant/types_neg.cc > new file mode 100644 > index 00000000000..5cd3d02154b > --- /dev/null > +++ b/libstdc++-v3/testsuite/20_util/variant/types_neg.cc > @@ -0,0 +1,17 @@ > +// { dg-do compile { target c++17 } } > + > +# include <variant> > + > +std::variant<> v0; // { dg-error "here" } > +// { dg-error "must have at least one alternative" "" { target *-*-* } 0 } > +std::variant<int, void> v1; // { dg-error "here" } > +std::variant<int, const void> v2; // { dg-error "here" } > +std::variant<int, int&> v3; // { dg-error "here" } > +std::variant<int, void()> v4; // { dg-error "here" } > +std::variant<int, int[]> v5; // { dg-error "here" } > +std::variant<int, int[1]> v6; // { dg-error "here" } > +// { dg-error "must be non-array object types" "" { target *-*-* } 0 } > + > +// All of variant's base classes are instantiated before checking any > +// static_assert, so we get lots of errors before the expected errors above. > +// { dg-excess-errors "" } > -- > 2.46.0 >
On Wed, 21 Aug 2024 at 10:03, Andrew Pinski <pinskia@gmail.com> wrote: > > On Wed, Aug 21, 2024 at 1:56 AM Jonathan Wakely <jwakely@redhat.com> wrote: > > > > Tested x86_64-linux. Pushed to trunk. > > > > Probably worth backporting too. It could potentially cause new errors > > for people using arrays in std::variant, but that's forbidden by the > > standard. > > It might be worth mentioning in porting_to guide just in case. You > never know since we have gotten bug reports about broken code that was > also rejected by clang/MSVC due to a change in GCC. Let's wait and see if it breaks anything when distros start building with the change. I don't expect any real world code to break, and if I'm right then there's no point documenting the change. For the backports maybe it makes sense to do it conditionally: #ifdef __STRICT_ANSI__ static_assert(((std::is_object_v<_Types> && !is_array_v<_Types>) && ...), "variant alternatives must be non-array object types"); #else static_assert((std::is_object_v<_Types> && ...), "variant alternatives must be object types"); #endif If you're asking for strict conformance, you shouldn't be using arrays in std::variant. This would avoid changing behaviour on release branches for non-strict modes.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 12108d07f0b..5fb7770d889 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -1457,10 +1457,8 @@ namespace __detail::__variant static_assert(sizeof...(_Types) > 0, "variant must have at least one alternative"); - static_assert(!(std::is_reference_v<_Types> || ...), - "variant must have no reference alternative"); - static_assert(!(std::is_void_v<_Types> || ...), - "variant must have no void alternative"); + static_assert(((std::is_object_v<_Types> && !is_array_v<_Types>) && ...), + "variant alternatives must be non-array object types"); using _Base = __detail::__variant::_Variant_base<_Types...>; diff --git a/libstdc++-v3/testsuite/20_util/variant/types_neg.cc b/libstdc++-v3/testsuite/20_util/variant/types_neg.cc new file mode 100644 index 00000000000..5cd3d02154b --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/variant/types_neg.cc @@ -0,0 +1,17 @@ +// { dg-do compile { target c++17 } } + +# include <variant> + +std::variant<> v0; // { dg-error "here" } +// { dg-error "must have at least one alternative" "" { target *-*-* } 0 } +std::variant<int, void> v1; // { dg-error "here" } +std::variant<int, const void> v2; // { dg-error "here" } +std::variant<int, int&> v3; // { dg-error "here" } +std::variant<int, void()> v4; // { dg-error "here" } +std::variant<int, int[]> v5; // { dg-error "here" } +std::variant<int, int[1]> v6; // { dg-error "here" } +// { dg-error "must be non-array object types" "" { target *-*-* } 0 } + +// All of variant's base classes are instantiated before checking any +// static_assert, so we get lots of errors before the expected errors above. +// { dg-excess-errors "" }