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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
