[PATCH] D50106: [libc++] Fix tuple assignment from types derived from a tuple-like

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

Suggested some ways to improve test coverage, and continued bikeshedding on the SFINAE ;) but there's no reason to hold this up AFAIC.



================
Comment at: libcxx/include/tuple:58
     tuple& operator=(const tuple&);
-    tuple&
-        operator=(tuple&&) noexcept(AND(is_nothrow_move_assignable<T>::value ...));
+    tuple& operator=(tuple&&) noexcept(AND(is_nothrow_move_assignable<T>::value ...));
     template <class... U>
----------------
Since this already isn't mimicking the syntax of http://eel.is/c++draft/tuple.tuple#tuple.assign-1 , I think you should say

    tuple& operator=(tuple&&) noexcept(is_nothrow_move_assignable_v<T> && ...);

but it definitely doesn't matter.


================
Comment at: libcxx/include/tuple:975
+
+    // EXTENSION
+    template<class _Up, size_t _Np, class = _EnableIf<
----------------
Perhaps in a followup patch: could we delineate exactly what code is part of this extension and put it all under an `#if _LIBCPP_TUPLE_ARRAY_ASSIGNMENT` or something, such that we could say "this is the exact extent of the extension code" and maybe even get customers to try compiling with `-D_LIBCPP_TUPLE_ARRAY_ASSIGNMENT=0` to see what breaks?

I'm assuming that our goal here is to deprecate and remove this extension over time. But even if we support it forever, I think `#if` would be nicer than the `//EXTENSION` comments.


================
Comment at: libcxx/include/tuple:992
+    // EXTENSION
+    template<class _Up, size_t _Np, class = void, class = _EnableIf<
+        _And<
----------------
Nitpick: Sent you a Slack suggesting how to eliminate this `class = void`.


================
Comment at: libcxx/test/libcxx/utilities/tuple/tuple.tuple/tuple.assign/const_array.pass.cpp:53
+        typedef std::array<int, 1> Array;
+        static_assert(!std::is_nothrow_assignable<Tuple&, Array const&>::value, "");
+    }
----------------
I would like to see this file combined with rvalue_array.pass.cpp, and also test the currently missing two value categories:

    static_assert(std::is_nothrow_assignable<Tuple&, Array&>::value, "");
    static_assert(std::is_nothrow_assignable<Tuple&, const Array&&>::value, "");

and also the negative cases:

    static_assert(!std::is_assignable<Tuple&, std::array<long,2>&>::value, "");
    static_assert(!std::is_assignable<Tuple&, std::array<long,2>&&>::value, "");
    static_assert(!std::is_assignable<Tuple&, const std::array<long,2>&>::value, "");
    static_assert(!std::is_assignable<Tuple&, const std::array<long,2>&&>::value, "");

    static_assert(!std::is_assignable<Tuple&, std::array<long,4>&>::value, "");
    static_assert(!std::is_assignable<Tuple&, std::array<long,4>&&>::value, "");
    static_assert(!std::is_assignable<Tuple&, const std::array<long,4>&>::value, "");
    static_assert(!std::is_assignable<Tuple&, const std::array<long,4>&&>::value, "");



================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair.pass.cpp:52
+        typedef std::pair<PotentiallyThrowingCopyAssignable, int> Pair;
+        static_assert(!std::is_nothrow_assignable<Tuple&, Pair const&>::value, "");
+    }
----------------
Don't just test for lack-of-nothrow-assignability; test for assignability-but-maythrow:

    static_assert(std::is_assignable<Tuple&, Pair const&>::value, "");
    static_assert(!std::is_nothrow_assignable<Tuple&, Pair const&>::value, "");

And as above, I'd prefer to see tests for `Pair&` and `Pair&&` and `const Pair&&` as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50106/new/

https://reviews.llvm.org/D50106