Page MenuHomePhabricator

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

Authored by huixie90 on Jan 4 2022, 1:04 PM.

Details

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&.
  1. 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).
  1. 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.
  1. 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
  1. 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.
  1. for const swap, I pretty much copy pasted the non-const version to make these overloads look consistent
  1. 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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Mar 24 2022, 1:29 PM
libcxx/include/tuple
516

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.

767

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.

785–787

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.

838

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

854–855

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.

1021–1023

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.

This revision now requires changes to proceed.Mar 24 2022, 1:29 PM

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
785–787

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
533

I

551–554

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 added inline comments.Mar 25 2022, 9:24 AM
libcxx/include/tuple
991–1005

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.

1102

Are we implementing this for std::array too?

EricWF added inline comments.Mar 25 2022, 2:15 PM
libcxx/include/tuple
1090

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
533

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

1691

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 commandeered this revision.Apr 30 2022, 9:22 AM
huixie90 added a reviewer: philnik.
huixie90 added a subscriber: huixie90.

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

huixie90 updated this revision to Diff 428911.May 12 2022, 5:25 AM
huixie90 marked 23 inline comments as done.

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

libcxx/include/tuple
838

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?

1090

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
1102

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

huixie90 edited the summary of this revision. (Show Details)May 18 2022, 3:27 AM
ldionne requested changes to this revision.Jun 7 2022, 8:21 AM

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.

770

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

775
776–777

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

779
785–787

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.

787–788

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.

838

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.

849

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

948

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

1084

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

1090

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

1102

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

1145

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.

1167–1174

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.

1299

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

1348

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.

This revision now requires changes to proceed.Jun 7 2022, 8:21 AM
huixie90 updated this revision to Diff 434989.Jun 7 2022, 4:36 PM
huixie90 marked 24 inline comments as done.

address review comments. refactored more assignment operators

libcxx/include/tuple
838

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?

849

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

1167–1174

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

ldionne accepted this revision.Jun 23 2022, 11:50 AM

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
838

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?

This revision is now accepted and ready to land.Jun 23 2022, 11:50 AM
huixie90 updated this revision to Diff 439498.Jun 23 2022, 12:28 PM
huixie90 marked 4 inline comments as done.

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

This revision was landed with ongoing or failed builds.Jun 23 2022, 1:29 PM
This revision was automatically updated to reflect the committed changes.
ldionne reopened this revision.Jun 23 2022, 2:00 PM
This revision is now accepted and ready to land.Jun 23 2022, 2:00 PM
huixie90 updated this revision to Diff 439539.Jun 23 2022, 2:25 PM

pull --rebase

ldionne accepted this revision.Jun 23 2022, 2:36 PM

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