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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Since `std::basic_common_reference` isn't exposition-only I would like an explicit test for it.



================
Comment at: libcxx/include/tuple:77
+
+template<class... TTypes, class... UTypes, template<class> class TQual, template<class> class UQual>
+  requires requires { typename tuple<common_reference_t<TQual<TTypes>, UQual<UTypes>>...>; }
----------------
Please add `// since C++23` for both new entries.


================
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> {
----------------
This suggestion is what the paper mandates and it matches the latest draft.


================
Comment at: libcxx/include/tuple:1127
+
+template <class... _TTypes, class... _UTypes>
+    requires requires { typename tuple<common_type_t<_TTypes, _UTypes>...>; }
----------------
Nit: I have a slight preference to use `_Tp` and `_Up` to match the other parts of the code.
(Since the previous function uses both Types and Qual I think the different naming there makes sense.)


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:203
+static_assert(is_same_v<common_reference_t<std::tuple<int&, const int&>, std::tuple<const int&, int>>,
+                        std::tuple<const int&, int>>);
+#endif
----------------
Can you add some tests with `volatile` and `const volatile`?


================
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>>);
----------------
I would prefer to use `std::common_type<...>::type` like the other test.
And likewise some `volatile` and `const volatile`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116538