* [PATCH] libstdc++/77641 fix std::variant for literal types
@ 2016-09-19 12:56 Jonathan Wakely
2016-09-19 20:31 ` Tim Shen
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2016-09-19 12:56 UTC (permalink / raw)
To: libstdc++, gcc-patches; +Cc: Tim Shen
[-- Attachment #1: Type: text/plain, Size: 837 bytes --]
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.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1431 bytes --]
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.
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{};
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-19 12:56 [PATCH] libstdc++/77641 fix std::variant for literal types Jonathan Wakely
@ 2016-09-19 20:31 ` Tim Shen
2016-09-20 18:40 ` Tim Shen
0 siblings, 1 reply; 9+ messages in thread
From: Tim Shen @ 2016-09-19 20:31 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
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.
--
Regards,
Tim Shen
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-19 20:31 ` Tim Shen
@ 2016-09-20 18:40 ` Tim Shen
2016-09-21 9:31 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Tim Shen @ 2016-09-20 18:40 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 779 bytes --]
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.
--
Regards,
Tim Shen
[-- Attachment #2: a.diff --]
[-- Type: application/octet-stream, Size: 2588 bytes --]
commit 9f29dcfbb779ccbafc5b7b36192ef6b0d4826ec9
Author: Tim Shen <timshen@google.com>
Date: Tue Sep 20 10:50:10 2016 -0700
2016-09-20 Tim Shen <timshen@google.com>
PR libstdc++/77641
* include/std/variant (_Variant_storage::_Variant_storage):
Change _Variant_storage's union to be default constructible.
* testsuite/20_util/variant/compile.cc: New test.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 7dbb533..013884b 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -296,15 +296,9 @@ namespace __variant
{
constexpr _Variant_storage() = default;
- template<typename... _Args>
- constexpr _Variant_storage(in_place_index_t<0>, _Args&&... __args)
- : _M_first(in_place<0>, forward<_Args>(__args)...)
- { }
-
- template<size_t _Np, typename... _Args,
- typename = enable_if_t<0 < _Np && _Np < sizeof...(_Rest) + 1>>
+ template<size_t _Np, typename... _Args>
constexpr _Variant_storage(in_place_index_t<_Np>, _Args&&... __args)
- : _M_rest(in_place<_Np - 1>, forward<_Args>(__args)...)
+ : _M_union(in_place<_Np>, forward<_Args>(__args)...)
{ }
~_Variant_storage() = default;
@@ -313,14 +307,27 @@ namespace __variant
_M_storage() const
{
return const_cast<void*>(
- static_cast<const void*>(std::addressof(_M_first._M_storage)));
+ static_cast<const void*>(std::addressof(_M_union._M_first._M_storage)));
}
- union
+ union _Union
{
+ constexpr _Union() {};
+
+ template<typename... _Args>
+ constexpr _Union(in_place_index_t<0>, _Args&&... __args)
+ : _M_first(in_place<0>, forward<_Args>(__args)...)
+ { }
+
+ template<size_t _Np, typename... _Args,
+ typename = enable_if_t<0 < _Np && _Np < sizeof...(_Rest) + 1>>
+ constexpr _Union(in_place_index_t<_Np>, _Args&&... __args)
+ : _M_rest(in_place<_Np - 1>, forward<_Args>(__args)...)
+ { }
+
_Uninitialized<__storage<_First>> _M_first;
_Variant_storage<_Rest...> _M_rest;
- };
+ } _M_union;
};
template<typename _Derived, bool __is_trivially_destructible>
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index b57d356..99f980a 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() { }
+ };
+
+ constexpr std::variant<X> v1 = X{};
+}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-20 18:40 ` Tim Shen
@ 2016-09-21 9:31 ` Jonathan Wakely
2016-09-22 6:32 ` Tim Shen
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2016-09-21 9:31 UTC (permalink / raw)
To: Tim Shen; +Cc: libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-21 9:31 ` Jonathan Wakely
@ 2016-09-22 6:32 ` Tim Shen
2016-09-22 10:16 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Tim Shen @ 2016-09-22 6:32 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On Wed, Sep 21, 2016 at 1:52 AM, Jonathan Wakely wrote:
> THanks, OK for trunk.
Committed.
Thanks!
--
Regards,
Tim Shen
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-22 6:32 ` Tim Shen
@ 2016-09-22 10:16 ` Jonathan Wakely
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2016-09-22 10:16 UTC (permalink / raw)
To: Tim Shen; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 191 bytes --]
On 21/09/16 20:23 -0700, Tim Shen wrote:
>On Wed, Sep 21, 2016 at 1:52 AM, Jonathan Wakely wrote:
>> THanks, OK for trunk.
>
>Committed.
This fixes the pretty printer.
Committed to trunk.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1968 bytes --]
commit 6b869af56fa80da5b746390ce5616ebebcc0bd5d
Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Thu Sep 22 10:06:41 2016 +0000
Update pretty printer for std::variant
* python/libstdcxx/v6/printers.py (StdVariantPrinter): Adjust for
recent change to _Variant_storage.
* testsuite/libstdc++-prettyprinters/cxx17.cc: Test variant with
reference type.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@240345 138bc75d-0d04-0410-961f-82ee72b054a4
diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py
index ac529dd..18c6f6b 100644
--- a/libstdc++-v3/python/libstdcxx/v6/printers.py
+++ b/libstdc++-v3/python/libstdcxx/v6/printers.py
@@ -994,7 +994,7 @@ class StdVariantPrinter(SingleObjContainerPrinter):
visualizer = None
else:
self.contained_type = alternatives[int(self.index)]
- addr = val['_M_first']['_M_storage'].address
+ addr = val['_M_union']['_M_first']['_M_storage'].address
contained_value = addr.cast(self.contained_type.pointer()).dereference()
visualizer = gdb.default_visualizer(contained_value)
super (StdVariantPrinter, self).__init__(contained_value, visualizer, 'array')
diff --git a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
index 42d79c0..09d79e9 100644
--- a/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
+++ b/libstdc++-v3/testsuite/libstdc++-prettyprinters/cxx17.cc
@@ -84,6 +84,9 @@ main()
variant<float, int, string_view> v4{ str };
// { dg-final { note-test v4 {std::variant<float, int, std::string_view> [index 2] = {"string"}} } }
+ variant<string_view&> vref{str};
+// { dg-final { note-test vref {std::variant<std::basic_string_view<char, std::char_traits<char> > &> [index 0] = {"string"}} } }
+
std::cout << "\n";
return 0; // Mark SPOT
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-22 8:39 ` Jonathan Wakely
@ 2016-09-22 8:47 ` Jonathan Wakely
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2016-09-22 8:47 UTC (permalink / raw)
To: Ville Voutilainen; +Cc: Tim Shen, libstdc++, gcc-patches
On 22/09/16 09:37 +0100, Jonathan Wakely wrote:
>On 22/09/16 09:45 +0300, Ville Voutilainen wrote:
>>This problem is not introduced by the latest patch, but it's something that
>>we should look at anyway. There's been recent discussion about what
>>assignments do with variants that hold references. Consider this:
>>
>>#include <variant>
>>
>>int main()
>>{
>> float f1 = 1.0f, f2 = 2.0f;
>>
>> std::variant<float&> v1(f1);
>>
>> v1 = f2; // #1
>>}
>
>It works if we add this to _Reference_storage:
>
> template<typename _Tp>
> _Reference_storage&
> operator=(_Tp&& __t)
> {
> _M_storage = std::forward<_Tp>(__t);
> return *this;
> }
>
>I'm not sure if that's the right fix.
Oh I missed Tim's patch for this that he already posted.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
2016-09-22 7:24 Ville Voutilainen
@ 2016-09-22 8:39 ` Jonathan Wakely
2016-09-22 8:47 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2016-09-22 8:39 UTC (permalink / raw)
To: Ville Voutilainen; +Cc: Tim Shen, libstdc++, gcc-patches
On 22/09/16 09:45 +0300, Ville Voutilainen wrote:
>This problem is not introduced by the latest patch, but it's something that
>we should look at anyway. There's been recent discussion about what
>assignments do with variants that hold references. Consider this:
>
>#include <variant>
>
>int main()
>{
> float f1 = 1.0f, f2 = 2.0f;
>
> std::variant<float&> v1(f1);
>
> v1 = f2; // #1
>}
It works if we add this to _Reference_storage:
template<typename _Tp>
_Reference_storage&
operator=(_Tp&& __t)
{
_M_storage = std::forward<_Tp>(__t);
return *this;
}
I'm not sure if that's the right fix.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++/77641 fix std::variant for literal types
@ 2016-09-22 7:24 Ville Voutilainen
2016-09-22 8:39 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: Ville Voutilainen @ 2016-09-22 7:24 UTC (permalink / raw)
To: Tim Shen, libstdc++, gcc-patches, Jonathan Wakely
This problem is not introduced by the latest patch, but it's something that
we should look at anyway. There's been recent discussion about what
assignments do with variants that hold references. Consider this:
#include <variant>
int main()
{
float f1 = 1.0f, f2 = 2.0f;
std::variant<float&> v1(f1);
v1 = f2; // #1
}
The line marked with #1 fails to compile before and after the latest
variant patch:
In file included from tony-variant1.cpp:1:0:
/usr/local/include/c++/7.0.0/variant: In instantiation of
‘std::enable_if_t<(((__exactly_once<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type> &&
is_constructible_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type, _Tp&&>) &&
is_assignable_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type&, _Tp&&>) && (!
is_same_v<typename std::decay<_Tp>::type, std::variant<_Types> >)),
std::variant<_Types>&> std::variant<_Types>::operator=(_Tp&&) [with
_Tp = float&; _Types = {float&};
std::enable_if_t<(((__exactly_once<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type> &&
is_constructible_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type, _Tp&&>) &&
is_assignable_v<typename
std::variant<_Types>::__to_type_impl<__accepted_index<_Tp&&>,
(__accepted_index<_Tp&&> < sizeof... (_Types))>::type&, _Tp&&>) && (!
is_same_v<typename std::decay<_Tp>::type, std::variant<_Types> >)),
std::variant<_Types>&> = std::variant<float&>&]’:
tony-variant1.cpp:9:9: required from here
/usr/local/include/c++/7.0.0/variant:1151:8: error: use of deleted
function ‘std::__detail::__variant::_Reference_storage<float&>&
std::__detail::__variant::_Reference_storage<float&>::operator=(std::__detail::__variant::_Reference_storage<float&>&&)’
*static_cast<__storage<__to_type<__index>>*>(this->_M_storage())
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
= forward<_Tp>(__rhs);
^~~~~~~~~~~~~~~~~~~~~
In file included from tony-variant1.cpp:1:0:
/usr/local/include/c++/7.0.0/variant:137:12: note:
‘std::__detail::__variant::_Reference_storage<float&>&
std::__detail::__variant::_Reference_storage<float&>::operator=(std::__detail::__variant::_Reference_storage<float&>&&)’
is implicitly deleted because the default definition would be
ill-formed:
struct _Reference_storage
^~~~~~~~~~~~~~~~~~
/usr/local/include/c++/7.0.0/variant:137:12: error: non-static
reference member ‘float&
std::__detail::__variant::_Reference_storage<float&>::_M_storage’,
can’t use default assignment operator
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-22 10:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-19 12:56 [PATCH] libstdc++/77641 fix std::variant for literal types Jonathan Wakely
2016-09-19 20:31 ` Tim Shen
2016-09-20 18:40 ` Tim Shen
2016-09-21 9:31 ` Jonathan Wakely
2016-09-22 6:32 ` Tim Shen
2016-09-22 10:16 ` Jonathan Wakely
2016-09-22 7:24 Ville Voutilainen
2016-09-22 8:39 ` Jonathan Wakely
2016-09-22 8:47 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).