[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/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>...> > {};
----------------
philnik wrote:
> Mordante wrote:
> > I noticed this while reviewing D116744, should this be disabled in C++23?
> Why?
> should this be disabled in C++23?

AIUI: No, but it might be helpful to the reader if we `s/Tuple/UserTuple/g`. This is basically replicating in userspace (for `Tuple`) what P2321 is now doing in library-space (for `std::tuple`); even in C++23, we want to keep testing that the userspace version still works.


================
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> {
----------------
philnik wrote:
> 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.
> I'll refactor lines 54-63 in another patch.

OK. Please also hit line 37 in that same 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.

I don't know much about the mazey relationships between `common_type`, `common_reference`, `basic_common_reference`, so I might be asking for nonsense. :) Also I might have said "test `common_type_t<...`" when I meant "test `common_reference_t<...`"?  But basically I'd like to see
- a test of variadic `common_{type,reference}_t<A,B,C>` where `common_type<A,B>` is due to the C++23-library-provided tuple specialization and `common_type<B,C>` is due to a user specialization
- a user specialization of `common_type` that involves `std::tuple<SomeUserDefinedType>`, proving that the C++23-library-provided partial specialization isn't interfering-with or ambiguating that user specialization

Does that make more sense?


================
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>>);
----------------
philnik wrote:
> 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.
I think these new static_asserts should go inside `main` just like lines 326–341; and they should be introduced with a comment `// P2321` just like line 325. (Although I would not complain if you were to indent line 325 in a more natural way, as a drive-by.) We should basically never add code //after// the closing brace of `main`.

And then for consistency I can see a good rationale for using `::type` and `::value` and `, "");` — although IMO that's less important than the other stuff I just mentioned, and I feel like @Mordante has asked to remove `, ""` from C++20 static_asserts in other places — anyway, count me as ambivalent on that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116538