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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:46
           template <class> class UQual>
 struct basic_common_reference< ::Tuple<Ts...>, ::Tuple<Us...>, TQual, UQual>
     : ::Tuple_helper<void, Tuple<TQual<Ts>...>, Tuple<UQual<Us>...> > {};
----------------
Mordante wrote:
> I noticed this while reviewing D116744, should this be disabled in C++23?
Why?


================
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> {
----------------
Quuxplusone wrote:
> 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>>`.
I'll refactor lines 54-63 in another patch.
I understand what you want to achieve with the common_reference asserts, but I don't know your intention with the common_type specializations.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp:345
+#if TEST_STD_VER > 20
+    static_assert(std::is_same_v<std::common_type_t<std::tuple<int>>, std::tuple<int>>);
+    static_assert(std::is_same_v<std::common_type_t<std::tuple<int>, std::tuple<long>>, std::tuple<long>>);
----------------
Mordante wrote:
> I would prefer to use `std::common_type<...>::type` like the other test.
> And likewise some `volatile` and `const volatile`.
I wouldn't do that, because the asserts are already very long and I don't think using `::type`instead of `_t` increases the readability in any way - the opposite even.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116538