[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:215
+}
+#endif
+
----------------
Please use `__x` and `__y` for consistency with lines 201–207.
Separately, don't you need a `noexcept`-spec similar to line 204?



================
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);
----------------
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.)


================
Comment at: libcxx/include/tuple:479
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    void swap(const __tuple_impl& __rhs) const noexcept(__all<is_nothrow_swappable_v<_Tp>...>::value) {
+        _VSTD::__swallow(__tuple_leaf<_Indx, _Tp>::swap(static_cast<const __tuple_leaf<_Indx, _Tp>&>(__rhs))...);
----------------
Since this is C++17-or-later, you can use `noexcept((is_nothrow_swappable_v<const _Tp&> && ...))`


================
Comment at: libcxx/include/tuple:746-749
+    template <class... _Up, __enable_if_t< requires {
+      sizeof...(_Up) == sizeof...(_Tp);
+      _EnableCopyFromOtherTuple<_Up...>::value;
+    }, int> = 0>
----------------
(1) Can't use `requires` because of `_LIBCPP_HAS_NO_CONCEPTS`.
(2) Don't want to use `requires` because what you actually meant is `sizeof...(_Up) == sizeof...(_Tp)` should be //true//, not just well-formed. I'd go so far as to claim that in libc++ contexts, mixing `enable_if_t` with `requires` is always wrong. (But maybe that's trivially true because if you were able to use `requires` you wouldn't need `enable_if_t` to express a constraint.)
(3) `__enable_if_t<sizeof...(_Up) == sizeof...(_Tp) && _EnableCopyFromOtherTuple<_Up...>::value>* = nullptr`


================
Comment at: libcxx/include/tuple:761
+    tuple(allocator_arg_t, const _Alloc& __alloc, tuple<_Up...>& __t) : __base_(allocator_arg_t(), __alloc, __t) {}
+#endif
+
----------------
Bikeshed: should `explicit(...)` be indented on lines 751 and 759? Personally I think not. It's not a "subordinate" clause in any sense; we don't normally indent the return type or `constexpr` when //that's// on a line by itself.
Significant: I assume you mean `explicit(!(...))`, right? Haven't checked the wording, but I would guess we want the ctor to be //implicit// only if all the types are convertible, not //explicit//. Please add a regression test for each of these `explicit`s; you can check by temporarily removing the `!` from one at a time, and making sure that some test fails, for each of them.


================
Comment at: libcxx/include/tuple:835
+    _LIBCPP_HIDE_FROM_ABI constexpr
+        explicit((is_convertible_v<const _Up&, _Tp> && ...))
+    tuple(const tuple<_Up...>&& __t) : __base_(_VSTD::move(__t)) {}
----------------
`&` should presumably be `&&`; please add a regression test.


================
Comment at: libcxx/include/tuple:968-985
+    template <class _Up1, class _Up2, class... _DependentTp>
+    struct _EnableMoveFromPair : _And<
         is_constructible<_FirstType<_DependentTp...>, _Up1>,
         is_constructible<_SecondType<_DependentTp...>, _Up2>,
-        is_convertible<_Up1, _FirstType<_DependentTp...> >, // explicit check
         is_convertible<_Up2, _SecondType<_DependentTp...> >
+    > {};
+
----------------
Again I haven't checked the wording (just cppreference), but this feels convoluted and thus wrong. It should be more like:
- EnableImplicitMoveFromPair if both types are implicitly convertible
- EnableExplicitMoveFromPair if both types are explicitly constructible //and not EnableImplicitMoveFromPair//

Which I think is how it was.
The only thing C++23 changes, AFAIK, is that now we've got two more ctors taking `pair&` and `const pair&&` in addition to C++11's `const pair&` and `pair&&`.

Actually I think your way of doing it below (lines 987-1012) is fine, except that you're misusing `requires` again — and you probably need to `#if _LIBCPP_STD_VER <= 20` the existing ctors from pair, so that in C++23 we get //only// the new conditionally-explicit four below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621