This is an archive of the discontinued LLVM Phabricator instance.

[libc++][P2321R2] Add specializations of basic_common_reference and common_type for tuple
ClosedPublic

Authored by philnik on Jan 3 2022, 6:32 AM.

Details

Summary

Add specializations of basic_common_reference and common_type for tuple

Diff Detail

Event Timeline

philnik requested review of this revision.Jan 3 2022, 6:32 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 6:32 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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

libcxx/include/tuple
77

Please add // since C++23 for both new entries.

1122

This suggestion is what the paper mandates and it matches the latest draft.

1127

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.)

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
179

Can you add some tests with volatile and const volatile?

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
355

I would prefer to use std::common_type<...>::type like the other test.
And likewise some volatile and const volatile.

Quuxplusone added inline comments.Jan 5 2022, 1:07 PM
libcxx/include/tuple
1122

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.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
18–21

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. :)

47–56

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>>.

Mordante added inline comments.Jan 6 2022, 8:54 AM
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
38–39

I noticed this while reviewing D116744, should this be disabled in C++23?

philnik updated this revision to Diff 398276.Jan 7 2022, 4:59 PM
philnik marked 6 inline comments as done.
  • reabsed onto main
  • Updated tests
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
38–39

Why?

47–56

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.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
355

I wouldn't do that, because the asserts are already very long and I don't think using ::typeinstead of _t increases the readability in any way - the opposite even.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
38–39

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.

47–56

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?

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
355

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.

philnik updated this revision to Diff 398379.Jan 8 2022, 2:39 PM
philnik marked 5 inline comments as done.
  • renamed Tuple to UserTuple
  • Update test
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
47–56

Yep, makes much more sense! But I think the two things are impossible to test separately.

Mordante accepted this revision as: Mordante.Jan 15 2022, 11:28 AM

Thanks for the ping. It seems I missed the updates for this patch. LGTM!

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp
38–39

Yes I initially was confused by the name and it's not a std::tuple.

ldionne accepted this revision.Jan 17 2022, 9:51 AM

LGTM with minor comment.

libcxx/docs/Status/Cxx2bPapers.csv
36

I'd suggest not including 14.0 here. We could put a number only when we complete the feature.

This revision is now accepted and ready to land.Jan 17 2022, 9:51 AM