[gcc r12-4888] libstdc++: Fix handling of const types in std::variant [PR102912]

[gcc r12-4888] libstdc++: Fix handling of const types in std::variant [PR102912]

[Jonathan Wakely]

https://gcc.gnu.org/g:7551a9957437f20254be41d396962b9ccc46cee6

commit r12-4888-g7551a9957437f20254be41d396962b9ccc46cee6
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Nov 3 13:51:45 2021 +0000

    libstdc++: Fix handling of const types in std::variant [PR102912]
    
    Prior to r12-4447 (implementing P2231R1 constexpr changes) we didn't
    construct the correct member of the union in __variant_construct_single,
    we just plopped an object in the memory occupied by the union:
    
      void* __storage = std::addressof(__lhs._M_u);
      using _Type = remove_reference_t<decltype(__rhs_mem)>;
      ::new (__storage) _Type(std::forward<decltype(__rhs_mem)>(__rhs_mem));
    
    We didn't care whether we had variant<int, const int>, we would just
    place an int (or const int) into the storage, and then set the _M_index
    to say which one it was.
    
    In the new constexpr-friendly code we use std::construct_at to construct
    the union object, which constructs the active member of the right type.
    But now we need to know exactly the right type. We have to distinguish
    between alternatives of type int and const int, and we have to be able
    to find a const int (or const std::string, as in the OP) among the
    alternatives. So my change from remove_reference_t<decltype(__rhs_mem)>
    to remove_cvref_t<_Up> was wrong. It strips the const from const int,
    and then we can't find the index of the const int alternative.
    
    But just using remove_reference_t doesn't work either. When the copy
    assignment operator of std::variant<int> uses __variant_construct_single
    it passes a const int& as __rhs_mem, but if we don't strip the const
    then we try to find const int among the alternatives, and *that* fails.
    Similarly for the copy constructor, which also uses a const int& as the
    initializer for a non-const int alternative.
    
    The root cause of the problem is that __variant_construct_single doesn't
    know the index of the type it's supposed to construct, and the new
    _Variant_storage::__index_of<_Type> helper doesn't work if __rhs_mem and
    the alternative being constructed have different const-qualification. We
    need to replace __variant_construct_single with something that knows the
    index of the alternative being constructed. All uses of that function do
    actually know the index, but that context is lost by the time we call
    __variant_construct_single. This patch replaces that function and
    __variant_construct, inlining their effects directly into the callers.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/102912
            * include/std/variant (_Variant_storage::__index_of): Remove.
            (__variant_construct_single): Remove.
            (__variant_construct): Remove.
            (_Copy_ctor_base::_Copy_ctor_base(const _Copy_ctor_base&)): Do
            construction directly instead of using __variant_construct.
            (_Move_ctor_base::_Move_ctor_base(_Move_ctor_base&&)): Likewise.
            (_Move_ctor_base::_M_destructive_move()): Remove.
            (_Move_ctor_base::_M_destructive_copy()): Remove.
            (_Copy_assign_base::operator=(const _Copy_assign_base&)): Do
            construction directly instead of using _M_destructive_copy.
            (variant::swap): Do construction directly instead of using
            _M_destructive_move.
            * testsuite/20_util/variant/102912.cc: New test.

Diff:
---
 libstdc++-v3/include/std/variant                 | 131 ++++++++---------------
 libstdc++-v3/testsuite/20_util/variant/102912.cc |  71 ++++++++++++
 2 files changed, 114 insertions(+), 88 deletions(-)

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 3da7dad1e82..63468af7012 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -467,10 +467,6 @@ namespace __variant
   template<typename... _Types>
     struct _Variant_storage<false, _Types...>
     {
-      template<typename _Tp>
-	static constexpr size_t __index_of
-	  = __detail::__variant::__index_of_v<_Tp, _Types...>;
-
       constexpr
       _Variant_storage()
       : _M_index(static_cast<__index_type>(variant_npos))
@@ -517,10 +513,6 @@ namespace __variant
   template<typename... _Types>
     struct _Variant_storage<true, _Types...>
     {
-      template<typename _Tp>
-	static constexpr size_t __index_of
-	  = __detail::__variant::__index_of_v<_Tp, _Types...>;
-
       constexpr
       _Variant_storage()
       : _M_index(static_cast<__index_type>(variant_npos))
@@ -560,35 +552,6 @@ namespace __variant
     using _Variant_storage_alias =
 	_Variant_storage<_Traits<_Types...>::_S_trivial_dtor, _Types...>;
 
-  template<typename _Tp, typename _Up>
-    _GLIBCXX20_CONSTEXPR
-    void
-    __variant_construct_single(_Tp&& __lhs, _Up&& __rhs_mem)
-    {
-      using _Type = __remove_cvref_t<_Up>;
-
-      if constexpr (!is_same_v<_Type, __variant_cookie>)
-	{
-	  using _Lhs = remove_reference_t<_Tp>;
-	  std::_Construct(std::__addressof(__lhs._M_u),
-			  in_place_index<_Lhs::template __index_of<_Type>>,
-			  std::forward<_Up>(__rhs_mem));
-	}
-    }
-
-  template<typename... _Types, typename _Tp, typename _Up>
-    _GLIBCXX20_CONSTEXPR
-    void
-    __variant_construct(_Tp&& __lhs, _Up&& __rhs)
-    {
-      __lhs._M_index = __rhs._M_index;
-      __variant::__raw_visit([&__lhs](auto&& __rhs_mem) mutable
-        {
-	  __variant_construct_single(std::forward<_Tp>(__lhs),
-	      std::forward<decltype(__rhs_mem)>(__rhs_mem));
-	}, __variant_cast<_Types...>(std::forward<_Up>(__rhs)));
-    }
-
   // The following are (Copy|Move) (ctor|assign) layers for forwarding
   // triviality and handling non-trivial SMF behaviors.
 
@@ -602,7 +565,15 @@ namespace __variant
       _Copy_ctor_base(const _Copy_ctor_base& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_copy_ctor)
       {
-	__variant_construct<_Types...>(*this, __rhs);
+	__variant::__raw_idx_visit(
+	  [this](auto&& __rhs_mem, auto __rhs_index) mutable
+	  {
+	    constexpr size_t __j = __rhs_index;
+	    if constexpr (__j != variant_npos)
+	      std::_Construct(std::__addressof(this->_M_u),
+			      in_place_index<__j>, __rhs_mem);
+	  }, __variant_cast<_Types...>(__rhs));
+	this->_M_index = __rhs._M_index;
       }
 
       _Copy_ctor_base(_Copy_ctor_base&&) = default;
@@ -631,27 +602,17 @@ namespace __variant
       _Move_ctor_base(_Move_ctor_base&& __rhs)
 	  noexcept(_Traits<_Types...>::_S_nothrow_move_ctor)
       {
-	__variant_construct<_Types...>(*this, std::move(__rhs));
+	__variant::__raw_idx_visit(
+	  [this](auto&& __rhs_mem, auto __rhs_index) mutable
+	  {
+	    constexpr size_t __j = __rhs_index;
+	    if constexpr (__j != variant_npos)
+	      std::_Construct(std::__addressof(this->_M_u),
+			      in_place_index<__j>,
+			      std::forward<decltype(__rhs_mem)>(__rhs_mem));
+	  }, __variant_cast<_Types...>(std::move(__rhs)));
+	this->_M_index = __rhs._M_index;
       }
-
-      template<typename _Up>
-	_GLIBCXX20_CONSTEXPR
-        void _M_destructive_move(unsigned short __rhs_index, _Up&& __rhs)
-        {
-	  this->_M_reset();
-	  __variant_construct_single(*this, std::forward<_Up>(__rhs));
-	  this->_M_index = __rhs_index;
-	}
-
-      template<typename _Up>
-	_GLIBCXX20_CONSTEXPR
-        void _M_destructive_copy(unsigned short __rhs_index, const _Up& __rhs)
-        {
-	  this->_M_reset();
-	  __variant_construct_single(*this, __rhs);
-	  this->_M_index = __rhs_index;
-	}
-
       _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;
@@ -662,24 +623,6 @@ namespace __variant
     {
       using _Base = _Copy_ctor_alias<_Types...>;
       using _Base::_Base;
-
-      template<typename _Up>
-	_GLIBCXX20_CONSTEXPR
-        void _M_destructive_move(unsigned short __rhs_index, _Up&& __rhs)
-        {
-	  this->_M_reset();
-	  __variant_construct_single(*this, std::forward<_Up>(__rhs));
-	  this->_M_index = __rhs_index;
-	}
-
-      template<typename _Up>
-	_GLIBCXX20_CONSTEXPR
-        void _M_destructive_copy(unsigned short __rhs_index, const _Up& __rhs)
-        {
-	  this->_M_reset();
-	  __variant_construct_single(*this, __rhs);
-	  this->_M_index = __rhs_index;
-	}
     };
 
   template<typename... _Types>
@@ -709,14 +652,19 @@ namespace __variant
 		    using __rhs_type = __remove_cvref_t<decltype(__rhs_mem)>;
 		    if constexpr (is_nothrow_copy_constructible_v<__rhs_type>
 			|| !is_nothrow_move_constructible_v<__rhs_type>)
-		      // The standard says this->emplace<__rhs_type>(__rhs_mem)
-		      // should be used here, but _M_destructive_copy is
-		      // equivalent in this case. Either copy construction
-		      // doesn't throw, so _M_destructive_copy gives strong
-		      // exception safety guarantee, or both copy construction
-		      // and move construction can throw, so emplace only gives
-		      // basic exception safety anyway.
-		      this->_M_destructive_copy(__rhs_index, __rhs_mem);
+		      {
+			// The standard says emplace<__rhs_index>(__rhs_mem)
+			// should be used here, but this is equivalent. Either
+			// copy construction doesn't throw, so we have strong
+			// exception safety guarantee, or both copy construction
+			// and move construction can throw, so emplace only
+			// gives basic exception safety anyway.
+			this->_M_reset();
+			std::_Construct(std::__addressof(this->_M_u),
+					in_place_index<__rhs_index>,
+					__rhs_mem);
+			this->_M_index = __rhs_index;
+		      }
 		    else
 		      __variant_cast<_Types...>(*this)
 			= variant<_Types...>(std::in_place_index<__rhs_index>,
@@ -1715,17 +1663,24 @@ namespace __variant
 		  }
 		else
 		  {
+		    constexpr size_t __j = __rhs_index;
 		    if (!this->valueless_by_exception()) [[__likely__]]
 		      {
 			auto __tmp(std::move(__rhs_mem));
 			__rhs = std::move(*this);
-			this->_M_destructive_move(__rhs_index,
-						  std::move(__tmp));
+			this->_M_reset();
+			std::_Construct(std::__addressof(this->_M_u),
+					in_place_index<__j>,
+					std::move(__tmp));
+			this->_M_index = __j;
 		      }
 		    else
 		      {
-			this->_M_destructive_move(__rhs_index,
-						  std::move(__rhs_mem));
+			this->_M_reset();
+			std::_Construct(std::__addressof(this->_M_u),
+					in_place_index<__j>,
+					std::move(__rhs_mem));
+			this->_M_index = __j;
 			__rhs._M_reset();
 		      }
 		  }
diff --git a/libstdc++-v3/testsuite/20_util/variant/102912.cc b/libstdc++-v3/testsuite/20_util/variant/102912.cc
new file mode 100644
index 00000000000..0dea138c5ba
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/variant/102912.cc
@@ -0,0 +1,71 @@
+// { dg-do compile { target c++17 } }
+#include <variant>
+
+void
+test01()
+{
+  struct X {
+    ~X() { } // non-trivial
+  };
+
+  std::variant<const int, const X> v;
+  auto vv = v;
+}
+
+#if __cpp_lib_variant >= 202106L // P2231R1 constexpr destruction in variant
+constexpr bool
+test02()
+{
+  struct Y {
+    constexpr ~Y() { } // non-trivial
+  };
+  using V = std::variant<int, const int, const Y, Y>;
+
+  V v1(std::in_place_index<1>, 1);
+  V vv1 = v1;
+  if (vv1.index() != v1.index())
+    return false;
+
+  V v2(std::in_place_index<2>);
+  V vv2 = v2;
+  if (vv2.index() != v2.index())
+    return false;
+
+  return true;
+}
+static_assert( test02() );
+
+constexpr bool
+test03()
+{
+  struct Y {
+    constexpr ~Y() { } // non-trivial
+  };
+  using V = std::variant<int, int, Y, Y>;
+
+  V v1(std::in_place_index<1>, 1);
+  V vv1 = v1;
+  if (vv1.index() != v1.index())
+    return false;
+  vv1 = v1;
+  if (vv1.index() != v1.index())
+    return false;
+  vv1 = std::move(v1);
+  if (vv1.index() != v1.index())
+    return false;
+
+  V v2(std::in_place_index<2>);
+  V vv2 = v2;
+  if (vv2.index() != v2.index())
+    return false;
+  vv2 = v2;
+  if (vv2.index() != v2.index())
+    return false;
+  vv2 = std::move(v2);
+  if (vv2.index() != v2.index())
+    return false;
+
+  return true;
+}
+static_assert( test03() );
+#endif