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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/tuple:1122
+template <class... _TTypes, class... _UTypes, template<class> class _TQual, template<class> class _UQual>
+    requires requires { typename tuple<common_reference_t<_TQual<_TTypes...>, _UQual<_UTypes...>>>; }
+struct basic_common_reference<tuple<_TTypes...>, tuple<_UTypes...>, _TQual, _UQual> {
----------------
Mordante wrote:
> This suggestion is what the paper mandates and it matches the latest draft.
And if the existing tests didn't catch this (which I guess they must not have): please add a regression test that would have caught this.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:21-24
 using std::common_reference;
 using std::common_reference_t;
 using std::is_same_v;
 using std::void_t;
----------------
If you want to make a preliminary NFC commit to eliminate all four of these `using`-declarations in favor of fully qualified names throughout, I'd approve it instantly. :)


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:54-63
 namespace std {
 template <>
 struct common_type<X2, Y2> {
   using type = Z2;
 };
 template <>
 struct common_type<Y2, X2> {
----------------
While you're here, I suggest rewriting lines 54–63 to not reopen namespace std:
```
template<> struct std::common_type<X2, Y2> { using type = Z2; }
template<> struct std::common_type<Y2, X2> { using type = Z2; }
```
And please add some tests involving them, e.g. IIUC
```
static_assert(std::is_same_v<std::common_reference_t<std::tuple<int, X2>, std::tuple<int, Y2>>, std::tuple<int, Z2>>);
static_assert(std::is_same_v<std::common_reference_t<std::tuple<int, X2>, std::tuple<int, Y2>>, std::tuple<int, Z2>>);
static_assert(!has_type<std::common_reference<std::tuple<int, const X2>, std::tuple<float, const Y2>>);
static_assert(!has_type<std::common_reference<std::tuple<int, X2>, std::tuple<float, Y2>>);
static_assert(!has_type<std::common_reference<std::tuple<int, X2>, int, X2>);
```
or whatever the results are supposed to be.

I haven't thought much about this, but it would probably be good to add a couple more specializations involving `tuple` to this test, e.g.
```
template<> struct std::common_type<X3, std::tuple<X2>> { using type = Z3; }
template<> struct std::common_type<std::tuple<X2>, X3> { using type = Z3; }
template<> struct std::common_type<X3, std::tuple<Y2>> { using type = Z3; }
template<> struct std::common_type<std::tuple<Y2>, X3> { using type = Z3; }
```
and then test e.g. `common_type_t<std::tuple<X2>, std::tuple<Y2>, X3>` and `common_type_t<std::tuple<X2>, std::tuple<Y2>, std::tuple<X3>>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116538