fix std::variant::swap for trivially-move-assignable types

Message ID 20180807142403.GO25399@redhat.com
State New
Headers show
Series
  • fix std::variant::swap for trivially-move-assignable types
Related show

Commit Message

Jonathan Wakely Aug. 7, 2018, 2:24 p.m.
This patch fixes the bug, but is it correct?

IIUC the _M_destructive_move effects don't depend on whether move
assignment is trivial, so should be defined the same way in both
specializations. It also looks like we can use it in the non-trivial
move assignment.

Should we define _M_destructive_move on _Move_ctor_base instead of
_Move_assign_base, so the duplication could be avoided?

Obviously this needs a ChangeLog entry and tests.

Comments

Jonathan Wakely Aug. 7, 2018, 2:29 p.m. | #1
On 07/08/18 15:24 +0100, Jonathan Wakely wrote:
>This patch fixes the bug, but is it correct?
>
>IIUC the _M_destructive_move effects don't depend on whether move
>assignment is trivial, so should be defined the same way in both
>specializations. It also looks like we can use it in the non-trivial
>move assignment.
>
>Should we define _M_destructive_move on _Move_ctor_base instead of
>_Move_assign_base, so the duplication could be avoided?

Or maybe into _Move_ctor_base as in the attached patch. That allows it
to be used in _Copy_assign_base, and means we can omit the try-catch
block when the move construction is trivial.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 66d878142a4..5dd00b05f1f 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -506,6 +506,20 @@ namespace __variant
 	  }
       }
 
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->~_Move_ctor_base();
+	__try
+	  {
+	    ::new (this) _Move_ctor_base(std::move(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
+
       _Move_ctor_base(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(_Move_ctor_base&&) = default;
@@ -516,6 +530,12 @@ namespace __variant
     {
       using _Base = _Copy_ctor_alias<_Types...>;
       using _Base::_Base;
+
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->~_Move_ctor_base();
+	::new (this) _Move_ctor_base(std::move(__rhs));
+      }
     };
 
   template<typename... _Types>
@@ -544,16 +564,7 @@ namespace __variant
 	else
 	  {
 	    _Copy_assign_base __tmp(__rhs);
-	    this->~_Copy_assign_base();
-	    __try
-	      {
-		::new (this) _Copy_assign_base(std::move(__tmp));
-	      }
-	    __catch (...)
-	      {
-		this->_M_index = variant_npos;
-		__throw_exception_again;
-	      }
+	    this->_M_destructive_move(std::move(__tmp));
 	  }
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
@@ -582,20 +593,6 @@ namespace __variant
       using _Base = _Copy_assign_alias<_Types...>;
       using _Base::_Base;
 
-      void _M_destructive_move(_Move_assign_base&& __rhs)
-      {
-	this->~_Move_assign_base();
-	__try
-	  {
-	    ::new (this) _Move_assign_base(std::move(__rhs));
-	  }
-	__catch (...)
-	  {
-	    this->_M_index = variant_npos;
-	    __throw_exception_again;
-	  }
-      }
-
       _Move_assign_base&
       operator=(_Move_assign_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
@@ -613,16 +610,7 @@ namespace __variant
 	else
 	  {
 	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->~_Move_assign_base();
-	    __try
-	      {
-		::new (this) _Move_assign_base(std::move(__tmp));
-	      }
-	    __catch (...)
-	      {
-		this->_M_index = variant_npos;
-		__throw_exception_again;
-	      }
+	    this->_M_destructive_move(std::move(__tmp));
 	  }
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
@@ -638,6 +626,20 @@ namespace __variant
     {
       using _Base = _Copy_assign_alias<_Types...>;
       using _Base::_Base;
+
+      void _M_destructive_move(_Move_assign_base&& __rhs)
+      {
+	this->~_Move_assign_base();
+	__try
+	  {
+	    ::new (this) _Move_assign_base(std::move(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
     };
 
   template<typename... _Types>
Ville Voutilainen Aug. 7, 2018, 2:57 p.m. | #2
On 7 August 2018 at 17:29, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 07/08/18 15:24 +0100, Jonathan Wakely wrote:
>>
>> This patch fixes the bug, but is it correct?
>>
>> IIUC the _M_destructive_move effects don't depend on whether move
>> assignment is trivial, so should be defined the same way in both
>> specializations. It also looks like we can use it in the non-trivial
>> move assignment.
>>
>> Should we define _M_destructive_move on _Move_ctor_base instead of
>> _Move_assign_base, so the duplication could be avoided?
>
>
> Or maybe into _Move_ctor_base as in the attached patch. That allows it
> to be used in _Copy_assign_base, and means we can omit the try-catch
> block when the move construction is trivial.
>

_Move_ctor_base seems fine to me. I plan to revamp our variant to
bring it up to the changes done before C++17
was done, to fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85517,
and I plan to do it shortly.
Jonathan Wakely Aug. 7, 2018, 7:13 p.m. | #3
On 07/08/18 17:57 +0300, Ville Voutilainen wrote:
>On 7 August 2018 at 17:29, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 07/08/18 15:24 +0100, Jonathan Wakely wrote:
>>>
>>> This patch fixes the bug, but is it correct?
>>>
>>> IIUC the _M_destructive_move effects don't depend on whether move
>>> assignment is trivial, so should be defined the same way in both
>>> specializations. It also looks like we can use it in the non-trivial
>>> move assignment.
>>>
>>> Should we define _M_destructive_move on _Move_ctor_base instead of
>>> _Move_assign_base, so the duplication could be avoided?
>>
>>
>> Or maybe into _Move_ctor_base as in the attached patch. That allows it
>> to be used in _Copy_assign_base, and means we can omit the try-catch
>> block when the move construction is trivial.
>>
>
>_Move_ctor_base seems fine to me. I plan to revamp our variant to
>bring it up to the changes done before C++17
>was done, to fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85517,
>and I plan to do it shortly.

OK, here's what I've committed after testing.
commit 15e32f819c03a268c11ad48e45c87e996be02479
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Aug 7 19:06:08 2018 +0100

    PR libstdc++/86874 fix std::variant::swap regression
    
            PR libstdc++/86874
            * include/std/variant (_Copy_ctor_base::_M_destructive_move): Define
            here instead of in _Move_assign_base.
            (_Copy_ctor_base<true, _Types...>::_M_destructive_move): Define.
            (_Copy_assign_base::operator=): Use _M_destructive_move when changing
            the contained value to another alternative.
            (_Move_assign_base::operator=): Likewise.
            (_Move_assign_base::_M_destructive_move): Remove.
            * testsuite/20_util/variant/86874.cc: New test.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 66d878142a4..2d86a704c63 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -506,6 +506,20 @@ namespace __variant
 	  }
       }
 
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->~_Move_ctor_base();
+	__try
+	  {
+	    ::new (this) _Move_ctor_base(std::move(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
+
       _Move_ctor_base(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(const _Move_ctor_base&) = default;
       _Move_ctor_base& operator=(_Move_ctor_base&&) = default;
@@ -516,6 +530,12 @@ namespace __variant
     {
       using _Base = _Copy_ctor_alias<_Types...>;
       using _Base::_Base;
+
+      void _M_destructive_move(_Move_ctor_base&& __rhs)
+      {
+	this->~_Move_ctor_base();
+	::new (this) _Move_ctor_base(std::move(__rhs));
+      }
     };
 
   template<typename... _Types>
@@ -538,22 +558,14 @@ namespace __variant
 	      {
 		static constexpr void (*_S_vtable[])(void*, void*) =
 		  { &__erased_assign<_Types&, const _Types&>... };
-		_S_vtable[__rhs._M_index](this->_M_storage(), __rhs._M_storage());
+		_S_vtable[__rhs._M_index](this->_M_storage(),
+					  __rhs._M_storage());
 	      }
 	  }
 	else
 	  {
 	    _Copy_assign_base __tmp(__rhs);
-	    this->~_Copy_assign_base();
-	    __try
-	      {
-		::new (this) _Copy_assign_base(std::move(__tmp));
-	      }
-	    __catch (...)
-	      {
-		this->_M_index = variant_npos;
-		__throw_exception_again;
-	      }
+	    this->_M_destructive_move(std::move(__tmp));
 	  }
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
@@ -582,20 +594,6 @@ namespace __variant
       using _Base = _Copy_assign_alias<_Types...>;
       using _Base::_Base;
 
-      void _M_destructive_move(_Move_assign_base&& __rhs)
-      {
-	this->~_Move_assign_base();
-	__try
-	  {
-	    ::new (this) _Move_assign_base(std::move(__rhs));
-	  }
-	__catch (...)
-	  {
-	    this->_M_index = variant_npos;
-	    __throw_exception_again;
-	  }
-      }
-
       _Move_assign_base&
       operator=(_Move_assign_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_assign)
@@ -613,16 +611,7 @@ namespace __variant
 	else
 	  {
 	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->~_Move_assign_base();
-	    __try
-	      {
-		::new (this) _Move_assign_base(std::move(__tmp));
-	      }
-	    __catch (...)
-	      {
-		this->_M_index = variant_npos;
-		__throw_exception_again;
-	      }
+	    this->_M_destructive_move(std::move(__tmp));
 	  }
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
diff --git a/libstdc++-v3/testsuite/20_util/variant/86874.cc b/libstdc++-v3/testsuite/20_util/variant/86874.cc
new file mode 100644
index 00000000000..b595f9581a1
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/86874.cc
@@ -0,0 +1,55 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++17" }
+// { dg-do run { target c++17 } }
+
+#include <variant>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::variant<std::monostate> v1, v2;
+  std::swap(v1, v2);
+}
+
+void
+test02()
+{
+  std::variant<int> v1{1}, v2{2};
+  std::swap(v1, v2);
+  VERIFY( std::get<0>(v1) == 2 );
+  VERIFY( std::get<0>(v2) == 1 );
+}
+
+void
+test03()
+{
+  std::variant<double, int> v1{1}, v2{2.3};
+  std::swap(v1, v2);
+  VERIFY( std::get<double>(v1) == 2.3 );
+  VERIFY( std::get<int>(v2) == 1 );
+}
+
+int
+main()
+{
+  test01();
+  test02();
+  test03();
+}

Patch

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 66d878142a4..7d691c487fd 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -613,16 +613,7 @@  namespace __variant
 	else
 	  {
 	    _Move_assign_base __tmp(std::move(__rhs));
-	    this->~_Move_assign_base();
-	    __try
-	      {
-		::new (this) _Move_assign_base(std::move(__tmp));
-	      }
-	    __catch (...)
-	      {
-		this->_M_index = variant_npos;
-		__throw_exception_again;
-	      }
+	    _M_destructive_move(std::move(__tmp));
 	  }
 	__glibcxx_assert(this->_M_index == __rhs._M_index);
 	return *this;
@@ -638,6 +629,19 @@  namespace __variant
     {
       using _Base = _Copy_assign_alias<_Types...>;
       using _Base::_Base;
+      void _M_destructive_move(_Move_assign_base&& __rhs)
+      {
+	this->~_Move_assign_base();
+	__try
+	  {
+	    ::new (this) _Move_assign_base(std::move(__rhs));
+	  }
+	__catch (...)
+	  {
+	    this->_M_index = variant_npos;
+	    __throw_exception_again;
+	  }
+      }
     };
 
   template<typename... _Types>