[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/tuple:307
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    int swap(const __tuple_leaf& __rhs) const noexcept(is_nothrow_swappable_v<__tuple_leaf>) {
+        _VSTD::swap(*this, __rhs);
----------------
Quuxplusone wrote:
> jloser wrote:
> > No action required, but it looks slightly odd that this uses `noexcept` directly whereas the one above uses `_NOEXCEPT_` .  Ditto for the new `swap` implemented below.
> I suspect this (and lines 381 and 479) should be `noexcept(is_nothrow_swappable_v<const __tuple_leaf&>)`. If I'm right, you should be able to (and thus, should) write a regression test that detects this mistake by smacking into the noexcept and `std::terminate`'ing when in fact it shouldn't.
> Here and line 380, please be consistent: either return `int` in both places for consistency with lines 299 and 373, or return `void` in both places because we can. (Ah, but if you return `void` then you'll have to use a fold-expression on line 480 instead of a `__swallow`. So return `int` for consistency.)
Even though I said `is_nothrow_swappable_v<const __tuple_leaf&>` above, it strikes me now that for consistency with line 300 we really want `is_nothrow_swappable_v<const __tuple_leaf>` (no ampersand).
Also, `s/__rhs/__t/g` here and line 383 and line 482.


================
Comment at: libcxx/include/tuple:760
+    _LIBCPP_HIDE_FROM_ABI constexpr
+        explicit(!(is_convertible_v<const _Up&, _Tp> && ...))
+    tuple(allocator_arg_t, const _Alloc& __alloc, tuple<_Up...>& __t) : __base_(allocator_arg_t(), __alloc, __t) {}
----------------
Right? (Please add a regression test.)


================
Comment at: libcxx/include/tuple:814
 
     // tuple(tuple<U...>&&) constructors (including allocator_arg_t variants)
     template <class ..._Up>
----------------
You're going to need the same `bool _Const` technique here. Please add a regression test that can detect the problem (e.g. via a type `A` where `A&&` is convertible to `B` but `B(const A&&)=delete`, and vice versa).


================
Comment at: libcxx/include/tuple:1095
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    const tuple& operator=(_If<__all<is_copy_assignable_v<const _Tp>...>::value, tuple, __nat> const& __tuple) const {
+        _VSTD::__memberwise_copy_assign(*this, __tuple, typename __make_tuple_indices<sizeof...(_Tp)>::type());
----------------
For comparison against line 1101, I think it'd be best to use `is_assignable<X&, Y>` for every one of these conditions, rather than switching from `is_assignable` to `is_copy_assignable`/`is_move_assignable` in special cases.
Chesterton's Fence on the use of `_And` rather than `__all`: I suspect there's a reason we want the short-circuiting behavior here. Please adjust line 1101 the same way. (Again, the guideline is "be consistent with pre-existing code unless you understand why the old code must be wrong.")


================
Comment at: libcxx/include/type_traits:506-509
+  template <class...>
+  using _FirstImpl = __nat;
+  template <class...>
+  using _SecondImpl = __nat;
----------------
Uh...? This completely changes the meaning of e.g. `_SecondType<int>` from "ill-formed" to "well-formed `__nat`." There's no way that's safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621