SFINAE on pair/tuple assignment operators 2729

[zoecarver]

Summary

This patch ensures that SFINAE is used to delete assignment operators in pair and tuple based on issue 2729.

[EricWF]

We should already have tests for this.

And you should only write fail.cpp tests when there is absolutely no other way to test the behavior.
In this case it can be tested with a passing test and type traits.

[zoecarver]

• remove fail tests
• update pass tests

[ldionne]

test/std/utilities/tuple/tuple.tuple/tuple.assign/copy.pass.cpp

115

I think you'd be testing the same thing if you said !std::is_assignable<T, P const>::value, but the intent might be clearer. What do you think?

You could also use T& instead to clear up any impression that this matters to the test (I had to think about it).

test/std/utilities/tuple/tuple.tuple/tuple.assign/move_pair.pass.cpp

40

I think it would be clearer to say NonMoveAssignable here, since this is really the property we're using.

[zoecarver]

• change name of helper struct
• update constness and value type of type trait

[zoecarver]

@ldionne updated based on your review. Does this look good now?

[ldionne]

test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp

76

Why don't you keep the old check for static_assert(!std::is_copy_assignable<P>::value, ""); too?

test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp

25

You should be able to add this to make it copy-assignable:

NonMoveAssignable& operator=(NonMoveAssignable const&) = default;

[zoecarver]

test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp

25

Hmm you're right but that unfortunatly fails the test !std::is_move_assignable<std::pair<int, NonMoveAssignable>>::value. I'll look into fixing it.

[zoecarver]

• Rebase off main
• Remove changes to unrelated tests

[ldionne]

So you added tests for std::tuple::operator=(std::pair const&) and std::tuple::operator=(std::pair&&), but not for:

• pair& operator=(const pair& p);
• template<class U, class V> pair& operator=(const pair<U, V>& p);
• pair& operator=(pair&& p) noexcept(see below);
• template<class U, class V> pair& operator=(pair<U, V>&& p);
• tuple& operator=(const tuple& u);
• tuple& operator=(tuple&& u) noexcept(see below);
• template <class... UTypes> tuple& operator=(const tuple<UTypes...>& u);
• template <class... UTypes> tuple& operator=(tuple<UTypes...>&& u);

The list is taken from the issue you linked. Is it because those constructors are already tested in these cases?

Also, the LWG issue says:

std::is_copy_assignable<std::pair<int, std::unique_ptr<int>>>::value is true, and should be false.

Can you please add a test with unique_ptr to the tests for the constructors that should support it? I'm a big fan of just copy-pasting simple things that we know should work properly and test that they do indeed work as intended. This avoids bad surprises.

[zoecarver]

• Rebase off main
• Add tests for the remaining overloads.
• Format changes with git-clang-format.

[zoecarver]

• Add test in const_pair.pass.cpp (missed in original pass).

[zoecarver]

@ldionne sorry about that. I don't know how I overlooked the majority of the issue. Anyway, I've made sure that all the overloads have a test now.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move.pass.cpp

108 ↗

(On Diff #325073)

This is just to mark where we have existing test coverage for this issue (to make it easier to review). I'll remove these comments when I commit.

[ldionne]

In D62454#2575490, @zoecarver wrote:

@ldionne sorry about that. I don't know how I overlooked the majority of the issue. Anyway, I've made sure that all the overloads have a test now.

Meh, no worries! Thanks a lot for adding the missing tests.

Shiiiiip it!

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/move.pass.cpp

108 ↗

(On Diff #325073)

Oh, ok, thanks a lot :-).

test/std/utilities/tuple/tuple.tuple/tuple.assign/copy.pass.cpp

struct CopyAssignable {
   CopyAssignable& operator=(CopyAssignable &&) = delete;
 };
 static_assert(std::is_copy_assignable<CopyAssignable>::value, "");
 struct MoveAssignable {
   MoveAssignable& operator=(MoveAssignable const&) = delete;
   MoveAssignable& operator=(MoveAssignable&&) = default;
 };
 
+struct CopyAssignableInt {
+    CopyAssignableInt& operator=(int&) { return *this; }
+};
+
 int main(int, char**)
 {
     {
         typedef std::tuple<> T;
         T t0;
         T t;
         t = t0;
     }
int main(int, char**)
     {
         using T = std::tuple<int, CopyAssignable>;
         static_assert(std::is_copy_assignable<T>::value, "");
     }
     {
         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>::value, "");
+    }
+    {
+        using T = std::tuple<int, MoveAssignable>;
+        using P = std::pair <int, MoveAssignable>;
+        static_assert(!std::is_assignable<T&, P&>::value, "");
+    }
 
   return 0;
 }

test/std/utilities/tuple/tuple.tuple/tuple.assign/move_pair.pass.cpp

 };
 
 struct D
     : B
 {
     explicit D(int i) : B(i) {}
 };
 
+struct CopyAssignable
+{
+  CopyAssignable& operator=(CopyAssignable const&) = default;
+  CopyAssignable& operator=(CopyAssignable &&) = delete;
+};
+
 int main(int, char**)
 {
     {
         typedef std::pair<long, std::unique_ptr<D>> T0;
         typedef std::tuple<long long, std::unique_ptr<B>> T1;
         T0 t0(2, std::unique_ptr<D>(new D(3)));
         T1 t1;
         t1 = std::move(t0);
         assert(std::get<0>(t1) == 2);
         assert(std::get<1>(t1)->id_ == 3);
     }
+    {
+        using T = std::tuple<int, CopyAssignable>;
+        using P = std::pair <int, CopyAssignable>;
+        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;
 }

test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp

 
 #include <utility>
 #include <memory>
 #include <cassert>
 
 #include "test_macros.h"
 
 
-struct NonAssignable {
-  NonAssignable& operator=(NonAssignable const&) = delete;
-  NonAssignable& operator=(NonAssignable&&) = delete;
+struct NonCopyAssignable {
+  NonCopyAssignable& operator=(NonCopyAssignable const&) = delete;
 };
 struct CopyAssignable {
   CopyAssignable() = default;
   CopyAssignable(CopyAssignable const&) = default;
   CopyAssignable& operator=(CopyAssignable const&) = default;
   CopyAssignable& operator=(CopyAssignable&&) = delete;
 };
 struct MoveAssignable {
int main(int, char**)
         int y2 = 300;
         P p1(x, std::move(y));
         P p2(x2, std::move(y2));
         p1 = p2;
         assert(p1.first == x2);
         assert(p1.second == y2);
     }
     {
-        using P = std::pair<int, NonAssignable>;
-        static_assert(!std::is_copy_assignable<P>::value, "");
+        using P = std::pair<int, NonCopyAssignable>;
+        static_assert(!std::is_move_assignable<P>::value, "");
     }
     {
         CountAssign::reset();
         using P = std::pair<CountAssign, CopyAssignable>;
         static_assert(std::is_copy_assignable<P>::value, "");
         P p;
         P p2;
         p = p2;

test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp

 
 #include <utility>
 #include <memory>
 #include <cassert>
 
 #include "test_macros.h"
 
 
-struct NonAssignable {
-  NonAssignable& operator=(NonAssignable const&) = delete;
-  NonAssignable& operator=(NonAssignable&&) = delete;
+struct NonMoveAssignable {
+  NonMoveAssignable& operator=(NonMoveAssignable&&) = delete;
 };
 struct CopyAssignable {
   CopyAssignable() = default;
   CopyAssignable& operator=(CopyAssignable const&) = default;
   CopyAssignable& operator=(CopyAssignable&&) = delete;
 };
 struct MoveAssignable {
   MoveAssignable() = default;
int main(int, char**)
         int y2 = 300;
         P p1(x, std::move(y));
         P p2(x2, std::move(y2));
         p1 = std::move(p2);
         assert(p1.first == x2);
         assert(p1.second == y2);
     }
     {
-        using P = std::pair<int, NonAssignable>;
+        using P = std::pair<int, NonMoveAssignable>;
         static_assert(!std::is_move_assignable<P>::value, "");
     }
     {
         // The move decays to the copy constructor
         CountAssign::reset();
         using P = std::pair<CountAssign, CopyAssignable>;
         static_assert(std::is_move_assignable<P>::value, "");
         P p;

www/cxx1z_status.html

<!--   <I>Note: "NAD" means that the issue was deemed "Not a defect"</I> -->
   <tr><td><a href="https://wg21.link/LWG2681">2681</a></td><td>filesystem::copy() cannot copy symlinks</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2682">2682</a></td><td>filesystem::copy() won't create a symlink to a directory</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2686">2686</a></td><td>Why is std::hash specialized for error_code, but not error_condition?</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2694">2694</a></td><td>Application of LWG 436 accidentally deleted definition of "facet"</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2696">2696</a></td><td>Interaction between make_shared and enable_shared_from_this is underspecified</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2699">2699</a></td><td>Missing restriction in [numeric.requirements]</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2712">2712</a></td><td>copy_file(from, to, ...) has a number of unspecified error conditions</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2722">2722</a></td><td>equivalent incorrectly specifies throws clause</td><td>Issaquah</td><td>Complete</td></tr>
-  <tr><td><a href="https://wg21.link/LWG2729">2729</a></td><td>Missing SFINAE on std::pair::operator=</td><td>Issaquah</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG2729">2729</a></td><td>Missing SFINAE on std::pair::operator=</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2732">2732</a></td><td>Questionable specification of path::operator/= and path::append</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2733">2733</a></td><td>[fund.ts.v2] gcd / lcm and bool</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2735">2735</a></td><td>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</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2736">2736</a></td><td>nullopt_t insufficiently constrained</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2738">2738</a></td><td>is_constructible with void types</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2739">2739</a></td><td>Issue with time_point non-member subtraction with an unsigned duration</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2740">2740</a></td><td>constexpr optional<T>::operator-></td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2742">2742</a></td><td>Inconsistent string interface taking string_view</td><td>Issaquah</td><td>Complete</td></tr>