[libcxx-commits] [PATCH] D131495: [libc++] implement "pair" section of P2321R2 `zip`

[Hui via Phabricator via libcxx-commits]

huixie90 marked an inline comment as done.
huixie90 added inline comments.


================
Comment at: libcxx/include/__utility/pair.h:211
+    explicit(!_CheckArgs::template __is_implicit<_U1&, _U2&>()) pair(pair<_U1, _U2>& __p)
+        noexcept((is_nothrow_constructible<first_type, _U1&>::value &&
+                  is_nothrow_constructible<second_type, _U2&>::value))
----------------
var-const wrote:
> The `noexcept` doesn't seem to be in the standard -- we're adding it for consistency with existing constructors, right?
that is right. None of the overload in the standard has `noexcept` and we added `noexcept` to the other overloads. I am just trying to be consistent with what we did in the past. Do you think it is reasonable?


================
Comment at: libcxx/include/__utility/pair.h:319
+    const pair& operator=(pair const& __p) const
+      noexcept(is_nothrow_copy_assignable_v<const first_type> &&
+               is_nothrow_copy_assignable_v<const second_type>)
----------------
var-const wrote:
> Same question here re. `noexcept`.
that is right. None of the overload in the standard has `noexcept` and we added `noexcept` to the other overloads. I am just trying to be consistent with what we did in the past. Do you think it is reasonable?


================
Comment at: libcxx/include/__utility/pair.h:514
+void swap(const pair<_T1, _T2>& __x, const pair<_T1, _T2>& __y)
+    _NOEXCEPT_(__is_nothrow_swappable<const _T1>::value &&
+               __is_nothrow_swappable<const _T2>::value)
----------------
var-const wrote:
> The `noexcept` specification is different from the standard -- looks like it's copied from the member version.
The standard is `noexcept(noexcept(x.swap(y)));` which is equivalent to the one here. The reason I wrote this way is to be consistent with what we did in the past (see the overload above this one). do you think we should change it?


================
Comment at: libcxx/include/utility:87
+    template <class U, class V>
+    constexpr explicit(see below) pair(const pair<U1, U2>&&);                    // since C++23
     template <class... Args1, class... Args2>
----------------
var-const wrote:
> This looks incorrect -- the template arguments are `U` and `V`, but the function arguments refer to `U1` and `U2`.
good catch


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/swap_member_const.pass.cpp:25
+concept MemberSwapNoexcept =
+    requires(T t1, T t2) {
+      { t1.swap(t2) } noexcept;
----------------
var-const wrote:
> Should this be `const T&`?
could be but I prefer this way. here `t1` and `t2` are lvalues of `T` and `T` could be non const or const. In my usage in this test, I choose `T` to be const. see line 35 - 38. What do you think? If I make it `const T&` here, I need to call this concept ConstMemberSwapNoexcept`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131495