[libcxx-commits] [PATCH] D117506: [libc++][P2321R2] Add specializations of basic_common_reference and common_type for pair

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

Looks good to me, modulo some nits and scope-creep followup ideas.



================
Comment at: libcxx/include/__utility/pair.h:323
+struct basic_common_reference<pair<_T1, _T2>, pair<_U1, _U2>, _TQual, _UQual> {
+    using type = pair<common_reference_t<_TQual<_T1>, _UQual<_U1>>, common_reference_t<_TQual<_T2>, _UQual<_U2>>>;
+};
----------------
Let's make this easy to compare to lines 320-321 at a glance.


================
Comment at: libcxx/include/__utility/pair.h:333-336
+#if _LIBCPP_STD_VER > 14
 template<class _T1, class _T2>
 pair(_T1, _T2) -> pair<_T1, _T2>;
 #endif
----------------
Let's keep the deduction guide for `pair` located right after class `pair`; both because it's more tightly "related" to the class than these partial specializations, and so that the library additions remain in chronological order (C++03, then C++17 deduction guide, then C++2b specializations).


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:204-205
+static_assert(std::is_same_v<std::common_reference_t<std::pair<int, long>, std::pair<long, int>>, std::pair<long, long>>);
+static_assert(std::is_same_v<std::common_reference_t<std::pair<int&, const int&>, std::pair<const int&, int>>,
+                             std::pair<const int&, int>>);
+static_assert(std::is_same_v<std::common_reference_t<std::pair<int&, volatile int&>, std::pair<volatile int&, int>>,
----------------
Let's pick one style and be consistent here: either one-line all of these like lines 202, 203, etc; or two-line them all like lines 204-205, 206-207, etc.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp:363
 
     static_assert(std::is_same_v<std::common_reference_t<A, std::tuple<B>,std::tuple<C>>, std::tuple<B>>);
+
----------------
Should this say `common_type_t` instead of `common_reference_t`? or what's this line doing here? Ditto below.
And/or, should this line be moved to `common_reference.compile.pass.cpp`?

Orthogonally, and probably in a separate NFC commit, can `common_type.pass.cpp` be renamed to `common_type.compile.pass.cpp` (and eliminate its `main`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117506