[libc++][nfc] SFINAE on pair/tuple assignment operators: LWG 2729.

zoecarver · zoecarver · commit 82c4701d4e7e · 2021-02-19T13:25:34.000-08:00
This patch ensures that SFINAE is used to delete assignment operators in pair and tuple based on issue 2729. Differential Review: https://reviews.llvm.org/D62454
diff --git a/libcxx/docs/Cxx1zStatusIssuesStatus.csv b/libcxx/docs/Cxx1zStatusIssuesStatus.csv
@@ -232,7 +232,7 @@
 "`2699 <https://wg21.link/LWG2699>`__","Missing restriction in [numeric.requirements]","Issaquah","|Complete|",""
 "`2712 <https://wg21.link/LWG2712>`__","copy_file(from, to, ...) has a number of unspecified error conditions","Issaquah","|Complete|",""
 "`2722 <https://wg21.link/LWG2722>`__","equivalent incorrectly specifies throws clause","Issaquah","|Complete|",""
-"`2729 <https://wg21.link/LWG2729>`__","Missing SFINAE on std::pair::operator=","Issaquah","",""
+"`2729 <https://wg21.link/LWG2729>`__","Missing SFINAE on std::pair::operator=","Issaquah","|Complete|",""
 "`2732 <https://wg21.link/LWG2732>`__","Questionable specification of path::operator/= and path::append","Issaquah","|Complete|",""
 "`2733 <https://wg21.link/LWG2733>`__","[fund.ts.v2] gcd / lcm and bool","Issaquah","|Complete|",""
 "`2735 <https://wg21.link/LWG2735>`__","std::abs(short), std::abs(signed char) and others should return int instead of double in order to be compatible with C++98 and C","Issaquah","|Complete|",""
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair.pass.cpp
@@ -17,6 +17,7 @@
 
 #include <tuple>
 #include <utility>
+#include <memory>
 #include <cassert>
 
 #include "test_macros.h"
@@ -32,6 +33,13 @@ int main(int, char**)
         assert(std::get<0>(t1) == 2);
         assert(std::get<1>(t1) == short('a'));
     }
+    {
+        // test that the implicitly generated copy assignment operator
+        // is properly deleted
+        using T = std::tuple<int, int>;
+        using P = std::tuple<std::unique_ptr<int>, std::unique_ptr<int>>;
+        static_assert(!std::is_assignable<T, const P &>::value, "");
+    }
 
   return 0;
 }
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_copy.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_copy.pass.cpp
@@ -34,6 +34,11 @@ struct D
     explicit D(int i = 0) : B(i) {}
 };
 
+struct NonAssignable {
+  NonAssignable& operator=(NonAssignable const&) = delete;
+  NonAssignable& operator=(NonAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
@@ -87,6 +92,12 @@ int main(int, char**)
         assert(std::get<0>(t) == 43);
         assert(&std::get<0>(t) == &x);
     }
+    {
+      using T = std::tuple<int, NonAssignable>;
+      using U = std::tuple<NonAssignable, int>;
+      static_assert(!std::is_assignable<T, U const&>::value, "");
+      static_assert(!std::is_assignable<U, T const&>::value, "");
+    }
 
   return 0;
 }
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_move.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_move.pass.cpp
@@ -45,6 +45,11 @@ struct E {
   }
 };
 
+struct NonAssignable {
+  NonAssignable& operator=(NonAssignable const&) = delete;
+  NonAssignable& operator=(NonAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
@@ -108,6 +113,12 @@ int main(int, char**)
         assert(std::get<0>(t) == 43);
         assert(&std::get<0>(t) == &x);
     }
+    {
+      using T = std::tuple<int, NonAssignable>;
+      using U = std::tuple<NonAssignable, int>;
+      static_assert(!std::is_assignable<T, U&&>::value, "");
+      static_assert(!std::is_assignable<U, T&&>::value, "");
+    }
 
   return 0;
 }
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/copy.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/copy.pass.cpp
@@ -35,6 +35,10 @@ struct MoveAssignable {
   MoveAssignable& operator=(MoveAssignable&&) = default;
 };
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
 int main(int, char**)
 {
     {
@@ -89,8 +93,8 @@ int main(int, char**)
         static_assert(!std::is_copy_assignable<T>::value, "");
     }
     {
-        using T = std::tuple<int, NonAssignable>;
-        static_assert(!std::is_copy_assignable<T>::value, "");
+      using T = std::tuple<int, NonAssignable>;
+      static_assert(!std::is_copy_assignable<T>::value, "");
     }
     {
         using T = std::tuple<int, CopyAssignable>;
@@ -100,6 +104,21 @@ int main(int, char**)
         using T = std::tuple<int, MoveAssignable>;
         static_assert(!std::is_copy_assignable<T>::value, "");
     }
+    {
+      using T = std::tuple<int, int, int>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T, P>::value, "");
+    }
+    { // test const requirement
+      using T = std::tuple<CopyAssignableInt, CopyAssignableInt>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T&, P const>::value, "");
+    }
+    {
+      using T = std::tuple<int, MoveAssignable>;
+      using P = std::pair<int, MoveAssignable>;
+      static_assert(!std::is_assignable<T&, P&>::value, "");
+    }
 
   return 0;
 }
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move.pass.cpp
@@ -105,8 +105,8 @@ int main(int, char**)
 
     }
     {
-        using T = std::tuple<int, NonAssignable>;
-        static_assert(!std::is_move_assignable<T>::value, "");
+      using T = std::tuple<int, NonAssignable>;
+      static_assert(!std::is_move_assignable<T>::value, "");
     }
     {
         using T = std::tuple<int, MoveAssignable>;
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move_pair.pass.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move_pair.pass.cpp
@@ -37,6 +37,11 @@ struct D
     explicit D(int i) : B(i) {}
 };
 
+struct NonMoveAssignable {
+  NonMoveAssignable& operator=(NonMoveAssignable const&) = default;
+  NonMoveAssignable& operator=(NonMoveAssignable&&) = delete;
+};
+
 int main(int, char**)
 {
     {
@@ -48,6 +53,16 @@ int main(int, char**)
         assert(std::get<0>(t1) == 2);
         assert(std::get<1>(t1)->id_ == 3);
     }
+    {
+      using T = std::tuple<int, NonMoveAssignable>;
+      using P = std::pair<int, NonMoveAssignable>;
+      static_assert(!std::is_assignable<T&, P&&>::value, "");
+    }
+    {
+      using T = std::tuple<int, int, int>;
+      using P = std::pair<int, int>;
+      static_assert(!std::is_assignable<T&, P&&>::value, "");
+    }
 
   return 0;
 }
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_const_pair_U_V.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_const_pair_U_V.pass.cpp
@@ -20,6 +20,12 @@
 #include "archetypes.h"
 #endif
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
+struct Unrelated {};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<int, short> P1;
@@ -58,6 +64,18 @@ TEST_CONSTEXPR_CXX20 bool test() {
     assert(p.second.value == -42);
   }
 #endif
+
+  { // test const requirement
+    using T = std::pair<CopyAssignableInt, CopyAssignableInt>;
+    using P = std::pair<int, int>;
+    static_assert(!std::is_assignable<T&, P const>::value, "");
+  }
+  {
+    using T = std::pair<int, Unrelated>;
+    using P = std::pair<Unrelated, int>;
+    static_assert(!std::is_assignable<T&, P&>::value, "");
+    static_assert(!std::is_assignable<P&, T&>::value, "");
+  }
   return true;
 }
 
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp
@@ -77,6 +77,10 @@ TEST_CONSTEXPR_CXX20 bool test() {
     using P = std::pair<int, ConstexprTestTypes::MoveAssignOnly>;
     static_assert(!std::is_copy_assignable<P>::value, "");
   }
+  {
+    using P = std::pair<int, std::unique_ptr<int> >;
+    static_assert(!std::is_copy_assignable<P>::value, "");
+  }
   {
     using P = std::pair<int, Incomplete&>;
     static_assert(!std::is_copy_assignable<P>::value, "");
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp
@@ -35,6 +35,11 @@ struct CountAssign {
   }
 };
 
+struct NotAssignable {
+  NotAssignable& operator=(NotAssignable const&) = delete;
+  NotAssignable& operator=(NotAssignable&&) = delete;
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<ConstexprTestTypes::MoveOnly, int> P;
@@ -83,6 +88,11 @@ TEST_CONSTEXPR_CXX20 bool test() {
     assert(p2.first.moved == 0);
     assert(p2.first.copied == 0);
   }
+  {
+    using T = std::pair<int, NotAssignable>;
+    using P = std::pair<int, NotAssignable>;
+    static_assert(!std::is_assignable<T, P&&>::value, "");
+  }
   return true;
 }
 
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair_U_V.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair_U_V.pass.cpp
@@ -40,6 +40,10 @@ struct CountAssign {
   }
 };
 
+struct CopyAssignableInt {
+  CopyAssignableInt& operator=(int&) { return *this; }
+};
+
 TEST_CONSTEXPR_CXX20 bool test() {
   {
     typedef std::pair<Derived, short> P1;
@@ -61,6 +65,12 @@ TEST_CONSTEXPR_CXX20 bool test() {
     assert(t.second.moved == 0);
     assert(t.second.copied == 0);
   }
+  { // test const requirement
+    using T = std::pair<CopyAssignableInt, CopyAssignableInt>;
+    using P = std::pair<int, int>;
+    static_assert(!std::is_assignable<T, P&&>::value, "");
+    static_assert(!std::is_assignable<P, T&&>::value, "");
+  }
   return true;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,11 @@ struct D`
`34`	`34`	`explicit D(int i = 0) : B(i) {}`
`35`	`35`	`};`
`36`	`36`
	`37`	`+struct NonAssignable {`
	`38`	`+ NonAssignable& operator=(NonAssignable const&) = delete;`
	`39`	`+ NonAssignable& operator=(NonAssignable&&) = delete;`
	`40`	`+};`
	`41`	`+`
`37`	`42`	`int main(int, char**)`
`38`	`43`	`{`
`39`	`44`	`{`
`@@ -87,6 +92,12 @@ int main(int, char**)`
`87`	`92`	`assert(std::get<0>(t) == 43);`
`88`	`93`	`assert(&std::get<0>(t) == &x);`
`89`	`94`	`}`
	`95`	`+ {`
	`96`	`+ using T = std::tuple<int, NonAssignable>;`
	`97`	`+ using U = std::tuple<NonAssignable, int>;`
	`98`	`+ static_assert(!std::is_assignable<T, U const&>::value, "");`
	`99`	`+ static_assert(!std::is_assignable<U, T const&>::value, "");`
	`100`	`+ }`
`90`	`101`
`91`	`102`	`return 0;`
`92`	`103`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,11 @@ struct E {`
`45`	`45`	`}`
`46`	`46`	`};`
`47`	`47`
	`48`	`+struct NonAssignable {`
	`49`	`+ NonAssignable& operator=(NonAssignable const&) = delete;`
	`50`	`+ NonAssignable& operator=(NonAssignable&&) = delete;`
	`51`	`+};`
	`52`	`+`
`48`	`53`	`int main(int, char**)`
`49`	`54`	`{`
`50`	`55`	`{`
`@@ -108,6 +113,12 @@ int main(int, char**)`
`108`	`113`	`assert(std::get<0>(t) == 43);`
`109`	`114`	`assert(&std::get<0>(t) == &x);`
`110`	`115`	`}`
	`116`	`+ {`
	`117`	`+ using T = std::tuple<int, NonAssignable>;`
	`118`	`+ using U = std::tuple<NonAssignable, int>;`
	`119`	`+ static_assert(!std::is_assignable<T, U&&>::value, "");`
	`120`	`+ static_assert(!std::is_assignable<U, T&&>::value, "");`
	`121`	`+ }`
`111`	`122`
`112`	`123`	`return 0;`
`113`	`124`	`}`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,10 @@ struct MoveAssignable {`
`35`	`35`	`MoveAssignable& operator=(MoveAssignable&&) = default;`
`36`	`36`	`};`
`37`	`37`
	`38`	`+struct CopyAssignableInt {`
	`39`	`+ CopyAssignableInt& operator=(int&) { return *this; }`
	`40`	`+};`
	`41`	`+`
`38`	`42`	`int main(int, char**)`
`39`	`43`	`{`
`40`	`44`	`{`
`@@ -89,8 +93,8 @@ int main(int, char**)`
`89`	`93`	`static_assert(!std::is_copy_assignable<T>::value, "");`
`90`	`94`	`}`
`91`	`95`	`{`
`92`		`- using T = std::tuple<int, NonAssignable>;`
`93`		`- static_assert(!std::is_copy_assignable<T>::value, "");`
	`96`	`+ using T = std::tuple<int, NonAssignable>;`
	`97`	`+ static_assert(!std::is_copy_assignable<T>::value, "");`
`94`	`98`	`}`
`95`	`99`	`{`
`96`	`100`	`using T = std::tuple<int, CopyAssignable>;`
`@@ -100,6 +104,21 @@ int main(int, char**)`
`100`	`104`	`using T = std::tuple<int, MoveAssignable>;`
`101`	`105`	`static_assert(!std::is_copy_assignable<T>::value, "");`
`102`	`106`	`}`
	`107`	`+ {`
	`108`	`+ using T = std::tuple<int, int, int>;`
	`109`	`+ using P = std::pair<int, int>;`
	`110`	`+ static_assert(!std::is_assignable<T, P>::value, "");`
	`111`	`+ }`
	`112`	`+ { // test const requirement`
	`113`	`+ using T = std::tuple<CopyAssignableInt, CopyAssignableInt>;`
	`114`	`+ using P = std::pair<int, int>;`
	`115`	`+ static_assert(!std::is_assignable<T&, P const>::value, "");`
	`116`	`+ }`
	`117`	`+ {`
	`118`	`+ using T = std::tuple<int, MoveAssignable>;`
	`119`	`+ using P = std::pair<int, MoveAssignable>;`
	`120`	`+ static_assert(!std::is_assignable<T&, P&>::value, "");`
	`121`	`+ }`
`103`	`122`
`104`	`123`	`return 0;`
`105`	`124`	`}`
Original file line number	Diff line number	Diff line change
`@@ -105,8 +105,8 @@ int main(int, char**)`
`105`	`105`
`106`	`106`	`}`
`107`	`107`	`{`
`108`		`- using T = std::tuple<int, NonAssignable>;`
`109`		`- static_assert(!std::is_move_assignable<T>::value, "");`
	`108`	`+ using T = std::tuple<int, NonAssignable>;`
	`109`	`+ static_assert(!std::is_move_assignable<T>::value, "");`
`110`	`110`	`}`
`111`	`111`	`{`
`112`	`112`	`using T = std::tuple<int, MoveAssignable>;`
Original file line number	Diff line number	Diff line change
`@@ -77,6 +77,10 @@ TEST_CONSTEXPR_CXX20 bool test() {`
`77`	`77`	`using P = std::pair<int, ConstexprTestTypes::MoveAssignOnly>;`
`78`	`78`	`static_assert(!std::is_copy_assignable<P>::value, "");`
`79`	`79`	`}`
	`80`	`+ {`
	`81`	`+ using P = std::pair<int, std::unique_ptr<int> >;`
	`82`	`+ static_assert(!std::is_copy_assignable<P>::value, "");`
	`83`	`+ }`
`80`	`84`	`{`
`81`	`85`	`using P = std::pair<int, Incomplete&>;`
`82`	`86`	`static_assert(!std::is_copy_assignable<P>::value, "");`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,11 @@ struct CountAssign {`
`35`	`35`	`}`
`36`	`36`	`};`
`37`	`37`
	`38`	`+struct NotAssignable {`
	`39`	`+ NotAssignable& operator=(NotAssignable const&) = delete;`
	`40`	`+ NotAssignable& operator=(NotAssignable&&) = delete;`
	`41`	`+};`
	`42`	`+`
`38`	`43`	`TEST_CONSTEXPR_CXX20 bool test() {`
`39`	`44`	`{`
`40`	`45`	`typedef std::pair<ConstexprTestTypes::MoveOnly, int> P;`
`@@ -83,6 +88,11 @@ TEST_CONSTEXPR_CXX20 bool test() {`
`83`	`88`	`assert(p2.first.moved == 0);`
`84`	`89`	`assert(p2.first.copied == 0);`
`85`	`90`	`}`
	`91`	`+ {`
	`92`	`+ using T = std::pair<int, NotAssignable>;`
	`93`	`+ using P = std::pair<int, NotAssignable>;`
	`94`	`+ static_assert(!std::is_assignable<T, P&&>::value, "");`
	`95`	`+ }`
`86`	`96`	`return true;`
`87`	`97`	`}`
`88`	`98`
Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,10 @@ struct CountAssign {`
`40`	`40`	`}`
`41`	`41`	`};`
`42`	`42`
	`43`	`+struct CopyAssignableInt {`
	`44`	`+ CopyAssignableInt& operator=(int&) { return *this; }`
	`45`	`+};`
	`46`	`+`
`43`	`47`	`TEST_CONSTEXPR_CXX20 bool test() {`
`44`	`48`	`{`
`45`	`49`	`typedef std::pair<Derived, short> P1;`
`@@ -61,6 +65,12 @@ TEST_CONSTEXPR_CXX20 bool test() {`
`61`	`65`	`assert(t.second.moved == 0);`
`62`	`66`	`assert(t.second.copied == 0);`
`63`	`67`	`}`
	`68`	`+ { // test const requirement`
	`69`	`+ using T = std::pair<CopyAssignableInt, CopyAssignableInt>;`
	`70`	`+ using P = std::pair<int, int>;`
	`71`	`+ static_assert(!std::is_assignable<T, P&&>::value, "");`
	`72`	`+ static_assert(!std::is_assignable<P, T&&>::value, "");`
	`73`	`+ }`
`64`	`74`	`return true;`
`65`	`75`	`}`
`66`	`76`