This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH 3/3] Fix condition for std::variant to be copy constructible


The standard says the std::variant copy constructor is defined as
deleted unless all alternative types are copy constructible, but we were
making it also depend on move constructible. Fix the condition and
enhance the tests to check the semantics with pathological copy-only
types (i.e. supporting copying but having deleted moves).
The enhanced tests revealed a regression in copy assignment for
non-trivial alternative types, where the assignment would not be
performed because the condition in the _Copy_assign_base visitor is
false: is_same_v<remove_reference_t<T&>, remove_reference_t<const T&>>.


Tested powerpc64le-linux.

I plan to commit all three of these patches later today, unless
somebody sees a problem with them.


commit a5a517df4933ffd0e6a08c42280c7d2ee0699904
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Apr 17 16:17:25 2019 +0100

    Fix condition for std::variant to be copy constructible
    
    The standard says the std::variant copy constructor is defined as
    deleted unless all alternative types are copy constructible, but we were
    making it also depend on move constructible. Fix the condition and
    enhance the tests to check the semantics with pathological copy-only
    types (i.e. supporting copying but having deleted moves).
    
    The enhanced tests revealed a regression in copy assignment for
    non-trivial alternative types, where the assignment would not be
    performed because the condition in the _Copy_assign_base visitor is
    false: is_same_v<remove_reference_t<T&>, remove_reference_t<const T&>>.
    
            * include/std/variant (__detail::__variant::_Traits::_S_copy_assign):
            Do not depend on whether all alternative types are move constructible.
            (__detail::__variant::_Copy_assign_base::operator=): Remove cv-quals
            from the operand when deciding whether to perform the assignment.
            * testsuite/20_util/variant/compile.cc (DeletedMoves): Define type
            with deleted move constructor and deleted move assignment operator.
            (default_ctor, copy_ctor, move_ctor, copy_assign, move_assign): Check
            behaviour of variants with DeletedMoves as an alternative.
            * testsuite/20_util/variant/run.cc (DeletedMoves): Define same type.
            (move_ctor, move_assign): Check that moving a variant with a
            DeletedMoves alternative falls back to copying instead of moving.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 22b0c3d5c22..e153363bbf3 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -279,7 +279,7 @@ namespace __variant
       static constexpr bool _S_move_ctor =
 	  (is_move_constructible_v<_Types> && ...);
       static constexpr bool _S_copy_assign =
-	  _S_copy_ctor && _S_move_ctor
+	  _S_copy_ctor
 	  && (is_copy_assignable_v<_Types> && ...);
       static constexpr bool _S_move_assign =
 	  _S_move_ctor
@@ -613,7 +613,7 @@ namespace __variant
 			  __variant::__get<__rhs_index>(*this);
 			if constexpr (is_same_v<
 				      remove_reference_t<decltype(__this_mem)>,
-				      remove_reference_t<decltype(__rhs_mem)>>)
+				      __remove_cvref_t<decltype(__rhs_mem)>>)
 			  __this_mem = __rhs_mem;
 		      }
 		  }
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index b67c98adf4a..5cc2a9460a9 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -63,6 +63,15 @@ struct MoveCtorOnly
 struct MoveCtorAndSwapOnly : MoveCtorOnly { };
 void swap(MoveCtorAndSwapOnly&, MoveCtorAndSwapOnly&) { }
 
+struct DeletedMoves
+{
+  DeletedMoves() = default;
+  DeletedMoves(const DeletedMoves&) = default;
+  DeletedMoves(DeletedMoves&&) = delete;
+  DeletedMoves& operator=(const DeletedMoves&) = default;
+  DeletedMoves& operator=(DeletedMoves&&) = delete;
+};
+
 struct nonliteral
 {
   nonliteral() { }
@@ -81,6 +90,7 @@ void default_ctor()
   static_assert(is_default_constructible_v<variant<string, string>>);
   static_assert(!is_default_constructible_v<variant<AllDeleted, string>>);
   static_assert(is_default_constructible_v<variant<string, AllDeleted>>);
+  static_assert(is_default_constructible_v<variant<DeletedMoves>>);
 
   static_assert(noexcept(variant<int>()));
   static_assert(!noexcept(variant<Empty>()));
@@ -93,6 +103,7 @@ void copy_ctor()
   static_assert(!is_copy_constructible_v<variant<AllDeleted, string>>);
   static_assert(is_trivially_copy_constructible_v<variant<int>>);
   static_assert(!is_trivially_copy_constructible_v<variant<std::string>>);
+  static_assert(is_trivially_copy_constructible_v<variant<DeletedMoves>>);
 
   {
     variant<int> a;
@@ -116,6 +127,7 @@ void move_ctor()
 {
   static_assert(is_move_constructible_v<variant<int, string>>);
   static_assert(!is_move_constructible_v<variant<AllDeleted, string>>);
+  static_assert(is_move_constructible_v<variant<int, DeletedMoves>>); // uses copy ctor
   static_assert(is_trivially_move_constructible_v<variant<int>>);
   static_assert(!is_trivially_move_constructible_v<variant<std::string>>);
   static_assert(!noexcept(variant<int, Empty>(declval<variant<int, Empty>>())));
@@ -157,6 +169,7 @@ void copy_assign()
   static_assert(!is_copy_assignable_v<variant<AllDeleted, string>>);
   static_assert(is_trivially_copy_assignable_v<variant<int>>);
   static_assert(!is_trivially_copy_assignable_v<variant<string>>);
+  static_assert(is_trivially_copy_assignable_v<variant<DeletedMoves>>);
   {
     variant<Empty> a;
     static_assert(!noexcept(a = a));
@@ -171,6 +184,7 @@ void move_assign()
 {
   static_assert(is_move_assignable_v<variant<int, string>>);
   static_assert(!is_move_assignable_v<variant<AllDeleted, string>>);
+  static_assert(is_move_assignable_v<variant<int, DeletedMoves>>); // uses copy assignment
   static_assert(is_trivially_move_assignable_v<variant<int>>);
   static_assert(!is_trivially_move_assignable_v<variant<string>>);
   {
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index 3ca375d374e..c0f48432ca1 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -57,6 +57,15 @@ struct AlwaysThrow
   bool operator>(const AlwaysThrow&) const { VERIFY(false); }
 };
 
+struct DeletedMoves
+{
+  DeletedMoves() = default;
+  DeletedMoves(const DeletedMoves&) = default;
+  DeletedMoves(DeletedMoves&&) = delete;
+  DeletedMoves& operator=(const DeletedMoves&) = default;
+  DeletedMoves& operator=(DeletedMoves&&) = delete;
+};
+
 void default_ctor()
 {
   variant<monostate, string> v;
@@ -80,6 +89,12 @@ void move_ctor()
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
   VERIFY(holds_alternative<string>(v));
+
+  variant<vector<int>, DeletedMoves> d{std::in_place_index<0>, {1, 2, 3, 4}};
+  // DeletedMoves is not move constructible, so this uses copy ctor:
+  variant<vector<int>, DeletedMoves> e(std::move(d));
+  VERIFY(std::get<0>(d).size() == 4);
+  VERIFY(std::get<0>(e).size() == 4);
 }
 
 void arbitrary_ctor()
@@ -137,6 +152,13 @@ void move_assign()
   VERIFY(holds_alternative<string>(u));
   VERIFY(get<string>(u) == "a");
   VERIFY(holds_alternative<string>(v));
+
+  variant<vector<int>, DeletedMoves> d{std::in_place_index<0>, {1, 2, 3, 4}};
+  variant<vector<int>, DeletedMoves> e;
+  // DeletedMoves is not move assignable, so this uses copy assignment:
+  e = std::move(d);
+  VERIFY(std::get<0>(d).size() == 4);
+  VERIFY(std::get<0>(e).size() == 4);
 }
 
 void arbitrary_assign()

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]