[libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

[huixie90]

Summary

Add const overloads to tuple swap, construction and assignment

Rational of the these tuple changes needed for zip

1. Constructor overloads that takes non-const lvalue reference, and const rvalue reference.
   
   Short answer: make zip_view::iterator model indirectly_readable.
   
   Long answer: indirectly_readable requires a common_reference between "lvalue reference to value_type" and reference, in zip's case, this could be tuple<Foo>& (lvalue reference to value_type) and tuple<Foo&> (reference). We need be able to construct tuple<Foo&> from tuple<Foo>&. pre-C++23, the tuple's converting copy constructors takes only const tuple<UTyles...>&. This won't work here because we cannot bind const Foo& to Foo&.

2. const member assignment operators
   
   Short answer: make zip_view::iterator model indirectly_writable
   
   Long answer: indirectly_writable requires the type const decltype(*it) to be assignable. The const there is to prevent cases such as sorting a transformed of range of prvalue string. It would have compiled without const because std::string{} = std::string{} is a valid expression. but const std::string{} = std::string{} is not. If the range is a range of references, adding const doesn't do anything. using T = int&, then const T is also int&. So this const in the concept is to ban range of prvalues. However, in zip case, although the reference is prvalue tuple, it is really just a proxy. If we have a tuple of references, we do want to model the concept because the underlying ranges are writable. So we need the const assignment operators if underlying types are const assignable (reference types are const assignable).

3. const swap
   
   because of the const assignment operator added above, the default std::swap's triple-move would lead to wrong result for const tuple<Foo&>

Now, here is the summary of the implementations.

1. for constructors that takes cvref variation of tuple<UTypes...>, there used to be two SFINAE helper _EnableCopyFromOtherTuple, _EnableMoveFromOtherTuple. And the implementations of these two helpers seem to slightly differ from the spec. But now, we need 4 variations. Instead of adding another two, this change refactored it to a single one _EnableCtrFromUTypesTuple, which directly maps to the spec without changing the C++11 behaviour. However, we need the helper __copy_cvref_t to get the type of std::get<i>(cvref tuple<Utypes...>) for different cvref, so I made __copy_cvref_t to be available in C++11.

2. for constructors that takes variations of std::pair, there used to be four helpers _EnableExplicitCopyFromPair, _EnableImplicitCopyFromPair, _EnableImplicitMoveFromPair, _EnableExplicitMoveFromPair. Instead of adding another four, this change refactored into two helper _EnableCtrFromPair and _BothImplicitlyConvertible. This also removes the need to use _nat

3. for const member assignment operator, since the requirement is very simple, I haven't refactored the old code but instead directly adding the new c++23 code.

4. for const swap, I pretty much copy pasted the non-const version to make these overloads look consistent

5. while doing these change, I found two of the old constructors wasn't marked constexpr for C++20 but they should. fixed them and added unit tests

[ldionne]

libcxx/include/tuple

525

Is there a reason for not doing it like we do for non-const types? Let's use the same approach for consistency, whatever that is.

776

This comment is now incorrect, I would say instead tuple([const] tuple<U...>&) constructors (including allocator_arg_t variants) or something like that.

The && comment below is also incorrect now -- please audit to make sure there aren't others too.

794–796

I don't like making these sorts of comments, but I don't understand why the indentation was changed. It used to be lined up to tabs.

847

I think what Eric's saying here is that you need to test for _EnableCopyFromPair *before* you test the two is_convertible conditions.

I think you could witness the difference by using a type that is *not* constructible from _Up, but where a conversion from _Up triggers a hard error. In that case, the fact that you changed the order of checking will cause you to instantiate the conversion operator before the constructor, and explode. If you had checked for the constructibility first, you'd have gotten a false answer and stopped there, instead of being tripped up by the hard error later on while trying to check whether the constructor should be explicit or not.

Please add tests for this -- we actually see these sorts of funky bad-behaved constructors and conversions in the wild. I remember breaking nlohmann::json with a change a couple years ago -- it wasn't fun. I'm not sure whether it ended up being our fault our theirs, but regardless, we want to be bullet proof to avoid breaking widely used libraries, because we look like jokers when that happens :-).

863–864

Don't you need to make changes here too to pass _Const and use __maybe_const<_Const, _Tp>&& instead? I think this should look like:

template <bool _Const, class ..._Up>
struct _EnableMoveFromOtherTuple : _And<
    _Not<is_same<tuple<_Tp...>, tuple<_Up...> > >,
    _Lazy<_Or,
        _BoolConstant<sizeof...(_Tp) != 1>,
        // _Tp and _Up are 1-element packs - the pack expansions look
        // weird to avoid tripping up the type traits in degenerate cases
        _Lazy<_And,
            _Not<is_convertible<__maybe_const<_Const, tuple<_Up>>&&, _Tp> >...,
            _Not<is_constructible<_Tp, __maybe_const<_Const, tuple<_Up>>&& > >...
        >
    >,
    is_constructible<_Tp, __maybe_const<_Const, _Up>&&>...
> { };

This should be observable if you have a type Foo in your tuple like this:

struct Foo {
  Foo(Other const&&);
  Foo(Other&&) = delete;
};

If you use is_constructible<_Tp, _Up> (where _Tp=Foo and _Up=Other), we'll prefer the Foo(Other&&) constructor and the answer will be "no". If we use my proposed version, it should use Foo(Other const&&) instead and be satisfied.

https://godbolt.org/z/b47ffv43P

Please add a test for this in all the constructors that take rvalue-refs.

1030–1032

Minor stylistic thing, but I'd do class _Alloc, class _U1, Class _U2 on the same line for consistency with the rest of the code in this file.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_cpp23.pass.cpp

16 ↗

(On Diff #417045)

Why does it fail on GCC?

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_cpp23.pass.cpp

1 ↗

(On Diff #417045)

Please follow the convention used for testing tuple constructors where we have one test file per constructor. It's a real pain, but it's the best way to ensure that we are testing everything. If you want to share code, that's fine, create a local .h in that directory.

13 ↗

(On Diff #417045)

Please use explicit(see-below), otherwise it looks like those are always implicit. Applies everywhere.

[EricWF]

Other than a couple breakages caused by this not working in C++14/C++11, I haven't seen anything blow up at Google, though I've yet to get a full test run in.
That is to say, I think this is good to land once the other inline comments are addressed.

libcxx/include/tuple

794–796

Actually, nevermind. __maybe_const is only available after C++17, but this compiles in older dialects. So we need to expose __maybe_const in older dialects.

libcxx/include/type_traits

447

I

465–468

It doesn't need to be substitution failure, but substitution failure is 1000x better than generating a bogus type and continuing on.
I'm not OK with this change.

In what cases are you trying to apply first/second type to a pack that doesn't have them?

[EricWF]

libcxx/include/tuple

1000–1014

If we don't have 2 types in the tuple, we shouldn't even be attempting this is_convertible check on the first one.

Don't instantiate any constructability/convertability checks in tuple if there are obvious reasons why the constructor evaluating them shouldn't be chosen, such as the arity.

1111

Are we implementing this for std::array too?

[EricWF]

libcxx/include/tuple

1099

I don't know that we need to do the _If trick here. Because it's not the true "copy assignment operator", we don't need to worry about the compiler generating one.

Try writing these like normal overloads with normal SFINAE.

libcxx/include/type_traits

447

I'm still against this. That's not how these meta-functions are meant to work. Also

3992

Use _If here to avoid adding more template instantiations.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_cpp23.pass.cpp

16 ↗

(On Diff #417045)

Oh, good catch.

I suspect it's because GCC rejects mutating mutable members in constant expressions.

Since we want *some* test coverage on GCC, I suggest we find a way to make at least part of this test pass under it.

[huixie90]

Had a chat with @philnik on discord and agreed to commandeer this change

[huixie90]

[libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

libcxx/include/tuple

847

I fixed the ordering issue from the previous version. But I am not sure If I understand the test case. IIUC, you are suggesting a case where
std::is_constructible_v<T, U> is false, but
std::is_convertible_v<U, T> is hard error

IIUC, convertible is a stricter version of constructible. if is_convertible_v has a hard error, the conversion operator/constructor that causes the error would also cause a hard error when evaluating is_constructible. or did I miss something?

1099

For this one, I tempt to keep _If as it is. few reasons:

1. This function does not naturally have a deduced type that can be used by SFINAE
2. I could introduce some template parameters, but that needs to have default value with Tp.... I am not sure if that is going to be very readable. Also, it will make it look like the overload that takes tuple<Us...>
3. The best way I think is to use requires but I am not sure if it is OK to use it as the rest of (C++20 bit) the file does not use it

1111

I am not sure if it is going to be very useful. The main use case (at least for the purpose of p2321r2) for these const overloads of assignment operators is that we can assign const tuple of reference types. (well, the const value types usually cannot be assigned to). I don't think std::array supports references

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_cpp23.pass.cpp

16 ↗

(On Diff #417045)

I disabled running these tests on compile time on gcc. but at least, it would run the tests at runtime with gcc

[ldionne]

I have some comments, but I really like the current state of this patch. This implements part of a new paper while refactoring a bunch of stuff and making it simpler at once, which is really great. Requesting changes because I'd like to take a look after comments have been applied, but I think this looks really good. Also, the tests are nicely structured and seem to cover all the required cases except for perhaps one minor detail mentioned in the comments. Thanks!

libcxx/include/tuple

30

Nit, but let's be consistent. Here and below.

779

Really small nitpick, but we generally use ctor instead of ctr for constructor (at least it's what I've seen so far).

784

785–786

What happens if you remove these conditions? The comments should explain what is the behavior that we are preserving.

788

794–796

I'm not requesting it, however if you'd like to add a regression test where for example _BoolConstant<sizeof...(_Tp) != 1> is false, but evaluating is_constructible<_Tp, _OtherTuple> is a hard error, I think it would catch this.

796–797

Mentioning _Not<T> does not instantiate T immediately, it only does when you do _Not<T>::whatever. Since you only do that lazily with my suggested edits to _And and _Or, you don't need _Lazy at this level anymore -- at least I think you don't, but our tests should tell you if I'm right.

847

I think I was thinking about a type having a conversion operator that triggers a hard error. However, it doesn't seem to apply anymore, but it would be nice to ensure that we don't trip over a type T where is_constructible<T, U>::value is false, but where is_convertible<T, U>::value is a hard error (by defining a conversion operator from U to T that causes a hard error). Let me know if you think this doesn't make sense, it might be impossible to actually trigger this issue but I'd need to dive deeper to be certain.

858

Do we need _SecondType and _FirstType in <type_traits> anymore? If not, let's remove them.

957

It's pretty obvious, but just to stay consistent with the other constructors above. This also applies to the other constructors below.

1093

We could use requires instead since we are in C++ > 20 and we only support compilers that support concepts.

1099

Per the above comment, we can use requires here and mark this original comment as done, IMO.

1111

Also, note that we don't support assigning tuple = array anymore. This was an extension, but it was removed.

1154

I like that you use _BoolConstant<sizeof...(_Tp) == sizeof...(_UTypes)> for self-documentation of the condition, however technically it is not needed because is_assignable<const _Tp&, const _UTypes&>... would SFINAE-away if that were not the case. I suggest you leave as-is for documentation purposes, I just wanted to point it out.

1176–1183

I think we should either use _EnableAssignFromPair with the const and the non-const overloads, or simplify _EnableAssignFromPair to always assume that _Const == true. Otherwise, this code looks buggy, even though it isn't.

1308

IMO it's not necessary for such a small block.

1357

Ditto.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_convert_copy.pass.cpp

35–36

66

81

You'll want to #include "test_macros.h" to get this.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair_copy.pass.cpp

16

Would it make sense to use T1, T2 instead of T0, T1? That way, indexing would match for Ti and Ui? If so, we should update the comments below.

42

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/types.h

2

4

This file is missing a license :)

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_const_move.pass.cpp

115

Let's use TEST_COMPILER_GCC.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_types.h

2

Same comment for license and header guard.

[huixie90]

address review comments. refactored more assignment operators

libcxx/include/tuple

847

hmm. IIUC, is_constructible also looks for conversion operator. If a conversion operator from U to T causes a hard error, both is_convertible and is_constructible would cause hard error. or, did I miss anything?

858

They were used in the noexcept specifier of some constructors and assignment operators. I went ahead refactoring these functions and _FirstType and _SecondType are no longer used

1176–1183

I did the former as refactoring the non-const allows me to completely remove _FirstType

[ldionne]

Thank you! This is a complicated patch but it ends up simplifying our tuple implementation quite a bit. Thanks for the great testing coverage. too.

libcxx/include/tuple

847

I think you're right.

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc_non_const_pair.pass.cpp

112

Can you please make sure you have newlines at the end of files?

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.swap/member_swap_const.pass.cpp

15

Why does it fail on GCC?

[huixie90]

added new lines to the end of every file and enable const swap runtime test for gcc

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc_non_const_pair.pass.cpp

112

done

libcxx/test/std/utilities/tuple/tuple.tuple/tuple.swap/member_swap_const.pass.cpp

15

good catch. gcc only fail when running the test at compile time because it doesn't allow mutable member in constexpr. but this test can run at time with gcc so I enable the runtime test for gcc now

[huixie90]

pull --rebase

[ldionne]

I looked at the diff before and after the rebase, still LGTM. Thanks!