| Message ID | 20160919123409.GA8225@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 19, 2016 at 5:34 AM, Jonathan Wakely <jwakely@redhat.com> wrote: > As noted in bugzilla PR 77641 I don't think is_literal_type is the > right condition for _Uninitialized, because a type can have a > non-trivial default constructor but still be literal, but that means > it can't be used in the union in _Variant_storage. > > I'm not sure if the right condition is is_literal && > is_trivially_default_constructible, or if this is enough: I believe that it's a "typo" from me - it should be is_trivially_destructible, not is_default_constructible (so that we can avoid using aligned_storage in the corresponding specialization). is_literal_type works, since literal types are trivially destructible. > > PR libstdc++/77641 > * include/std/variant (__detail::__variant::_Uninitialized): Check > is_trivially_default_constructible_v instead of is_literal_type_v. > * testsuite/20_util/variant/compile.cc: Test literal type with > non-trivial default constructor. > > Tim, are there case that this doesn't handle, where we need is_literal > as well? (bear in mind that is_trivially_default_constructible also > depends on trivially destructible). I checked for is_default_constructible, and all other sites are appropriate.
On Mon, Sep 19, 2016 at 1:06 PM, Tim Shen <timshen@google.com> wrote: > I believe that it's a "typo" from me - it should be > is_trivially_destructible, not is_default_constructible (so that we > can avoid using aligned_storage in the corresponding specialization). > is_literal_type works, since literal types are trivially destructible. Sorry I misunderstood your patch. The underlying problem is that _Variant_storage wasn't always default constructible, but it should be. Jon, your fix doesn't fix the following constexpr variation of your test case: struct X { constexpr X() { } }; constexpr std::variant<X> v1 = X{}; So I have a patch here to make _Variant_storage's internal union default constructible. Tested on x86_64-linux-gnu.
On 20/09/16 11:03 -0700, Tim Shen wrote: >On Mon, Sep 19, 2016 at 1:06 PM, Tim Shen <timshen@google.com> wrote: >> I believe that it's a "typo" from me - it should be >> is_trivially_destructible, not is_default_constructible (so that we >> can avoid using aligned_storage in the corresponding specialization). >> is_literal_type works, since literal types are trivially destructible. > >Sorry I misunderstood your patch. > >The underlying problem is that _Variant_storage wasn't always default >constructible, but it should be. > >Jon, your fix doesn't fix the following constexpr variation of your test case: > struct X { > constexpr X() { } > }; > > constexpr std::variant<X> v1 = X{}; > >So I have a patch here to make _Variant_storage's internal union >default constructible. > >Tested on x86_64-linux-gnu. THanks, OK for trunk.
On Wed, Sep 21, 2016 at 1:52 AM, Jonathan Wakely wrote:
> THanks, OK for trunk.
Committed.
Thanks!
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 7dbb533..6c090f6 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -168,7 +168,7 @@ namespace __variant template<typename _Type> using __storage = typename __storage_type<_Type>::type; - template<typename _Type, bool __is_literal = std::is_literal_type_v<_Type>> + template<typename _Type, bool = is_trivially_default_constructible_v<_Type>> struct _Uninitialized; template<typename _Type> diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc index b57d356..3042a2e 100644 --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc @@ -403,3 +403,12 @@ void test_void() v = 3; v = "asdf"; } + +void test_pr77641() +{ + struct X { + constexpr X() { } + }; + + std::variant<X> v1 = X{}; +}
As noted in bugzilla PR 77641 I don't think is_literal_type is the right condition for _Uninitialized, because a type can have a non-trivial default constructor but still be literal, but that means it can't be used in the union in _Variant_storage. I'm not sure if the right condition is is_literal && is_trivially_default_constructible, or if this is enough: PR libstdc++/77641 * include/std/variant (__detail::__variant::_Uninitialized): Check is_trivially_default_constructible_v instead of is_literal_type_v. * testsuite/20_util/variant/compile.cc: Test literal type with non-trivial default constructor. Tim, are there case that this doesn't handle, where we need is_literal as well? (bear in mind that is_trivially_default_constructible also depends on trivially destructible). Tested powerpc64le-linux with no failures. commit 0d5f60751145f81d914d7411e0f4d79546e06fed Author: Jonathan Wakely <jwakely@redhat.com> Date: Mon Sep 19 13:18:07 2016 +0100 libstdc++/77641 fix std::variant for literal types PR libstdc++/77641 * include/std/variant (__detail::__variant::_Uninitialized): Check is_trivially_default_constructible_v instead of is_literal_type_v. * testsuite/20_util/variant/compile.cc: Test literal type with non-trivial default constructor.