This is an archive of the discontinued LLVM Phabricator instance.

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

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



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

philnik requested review of this revision.Jan 4 2022, 1:04 PM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 1:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

P2321R2 "zip" is a big paper, so it makes sense to chunk it up into smaller PRs like this. But I think this PR is too small. The notions of "const-swappable" and "const-assignable" seem very tightly coupled to each other. Therefore I'm pretty confident in asserting that whatever PR implements const-swap for T, should also implement const-assign for T.

It might even make sense to implement all the tuple stuff and all the pair stuff together in this same PR, although I'm not at all confident of that.

struct S {
  int *calls;
  friend constexpr void swap(S& a, S& b) {
    *calls += 1;
struct CS {
  int *calls;
  friend constexpr void swap(const CS& a, const CS& b) {
    *calls += 1;

And then test various mixtures of S, CS, and S&; and for the behavior tests, I wouldn't focus on the ultimate effects (like a.i == 1) but instead on whether they're actually calling the user's ADL swap function correctly:

static_assert( std::is_swappable_v<std::tuple<>>);
static_assert( std::is_swappable_v<std::tuple<S>>);
static_assert( std::is_swappable_v<std::tuple<CS>>);
static_assert( std::is_swappable_v<std::tuple<S&>>);
static_assert( std::is_swappable_v<std::tuple<CS, S>>);
static_assert( std::is_swappable_v<std::tuple<CS, S&>>);
static_assert( std::is_swappable_v<const std::tuple<>>);
static_assert(!std::is_swappable_v<const std::tuple<S>>);
static_assert( std::is_swappable_v<const std::tuple<CS>>);
static_assert( std::is_swappable_v<const std::tuple<S&>>);
static_assert(!std::is_swappable_v<const std::tuple<CS, S>>);
static_assert( std::is_swappable_v<const std::tuple<CS, S&>>);
  int cs_calls = 0;
  int s_calls = 0;
  S s1{s_calls};
  S s2{s_calls};
  const std::tuple<CS, S&> t1 = { CS{cs_calls}, s1 };
  const std::tuple<CS, S&> t2 = { CS{cs_calls}, s2 };
  swap(t1, t2);
  assert(cs_calls == 1);
  assert(s_calls == 1);

And then I guess there should be some tests for is_nothrow_swappable, too; and for a type that isn't swappable at all.

I didn't have a close look, I prefer to see a version with @Quuxplusone's points addressed.

Can you update the C++2b status page to have a partial implementation of P2321R2?

Since P2321 is a large paper I think it makes sense to have a separate review with a new status pages so we can keep track of the part that are and aren't implemented. Can you take care of that?

I just see D116538 already updates the status page, so discard that request.

I can add a status page. Is there any way to host the website locally for checking that it looks as expected?

You can generate the .html files locally and then look at the output, no webserver required.
This requires Sphinx on your system.
The build instructions are:

  • reconfigure cmake with -DLLVM_ENABLE_SPHINX=ON
  • build the target docs-libcxx-html

When it works it gives the path where the output is stored. This should be ${BUILDDIR}/libcxx/docs/html

If you need further assistance, best ask on Discord.

philnik updated this revision to Diff 404414.Jan 30 2022, 4:07 PM
  • Updated test
  • Fixed swap
  • Add constructor and assignment overloads

I know the tests aren't covering everything. First of all I want to know if I understood the standard correctly.

philnik retitled this revision from [libc++][P2321R2] Add const overloads to tuple swap to [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment.Jan 30 2022, 4:08 PM
philnik edited the summary of this revision. (Show Details)
philnik added a reviewer: var-const.
jloser added a subscriber: jloser.Jan 30 2022, 4:17 PM
jloser added inline comments.

No action required, but it looks slightly odd that this uses noexcept directly whereas the one above uses _NOEXCEPT_ . Ditto for the new swap implemented below.


Please use __x and __y for consistency with lines 201–207.
Separately, don't you need a noexcept-spec similar to line 204?


I suspect this (and lines 381 and 479) should be noexcept(is_nothrow_swappable_v<const __tuple_leaf&>). If I'm right, you should be able to (and thus, should) write a regression test that detects this mistake by smacking into the noexcept and std::terminate'ing when in fact it shouldn't.
Here and line 380, please be consistent: either return int in both places for consistency with lines 299 and 373, or return void in both places because we can. (Ah, but if you return void then you'll have to use a fold-expression on line 480 instead of a __swallow. So return int for consistency.)


Since this is C++17-or-later, you can use noexcept((is_nothrow_swappable_v<const _Tp&> && ...))


(1) Can't use requires because of _LIBCPP_HAS_NO_CONCEPTS.
(2) Don't want to use requires because what you actually meant is sizeof...(_Up) == sizeof...(_Tp) should be true, not just well-formed. I'd go so far as to claim that in libc++ contexts, mixing enable_if_t with requires is always wrong. (But maybe that's trivially true because if you were able to use requires you wouldn't need enable_if_t to express a constraint.)
(3) __enable_if_t<sizeof...(_Up) == sizeof...(_Tp) && _EnableCopyFromOtherTuple<_Up...>::value>* = nullptr


Bikeshed: should explicit(...) be indented on lines 751 and 759? Personally I think not. It's not a "subordinate" clause in any sense; we don't normally indent the return type or constexpr when that's on a line by itself.
Significant: I assume you mean explicit(!(...)), right? Haven't checked the wording, but I would guess we want the ctor to be implicit only if all the types are convertible, not explicit. Please add a regression test for each of these explicits; you can check by temporarily removing the ! from one at a time, and making sure that some test fails, for each of them.


& should presumably be &&; please add a regression test.


Again I haven't checked the wording (just cppreference), but this feels convoluted and thus wrong. It should be more like:

  • EnableImplicitMoveFromPair if both types are implicitly convertible
  • EnableExplicitMoveFromPair if both types are explicitly constructible and not EnableImplicitMoveFromPair

Which I think is how it was.
The only thing C++23 changes, AFAIK, is that now we've got two more ctors taking pair& and const pair&& in addition to C++11's const pair& and pair&&.

Actually I think your way of doing it below (lines 987-1012) is fine, except that you're misusing requires again — and you probably need to #if _LIBCPP_STD_VER <= 20 the existing ctors from pair, so that in C++23 we get only the new conditionally-explicit four below.

philnik updated this revision to Diff 406299.Feb 6 2022, 4:11 PM
philnik marked 9 inline comments as done.
  • Address comments
Quuxplusone added inline comments.Feb 7 2022, 7:42 AM

Even though I said is_nothrow_swappable_v<const __tuple_leaf&> above, it strikes me now that for consistency with line 300 we really want is_nothrow_swappable_v<const __tuple_leaf> (no ampersand).
Also, s/__rhs/__t/g here and line 383 and line 482.


Right? (Please add a regression test.)


You're going to need the same bool _Const technique here. Please add a regression test that can detect the problem (e.g. via a type A where A&& is convertible to B but B(const A&&)=delete, and vice versa).


For comparison against line 1101, I think it'd be best to use is_assignable<X&, Y> for every one of these conditions, rather than switching from is_assignable to is_copy_assignable/is_move_assignable in special cases.
Chesterton's Fence on the use of _And rather than __all: I suspect there's a reason we want the short-circuiting behavior here. Please adjust line 1101 the same way. (Again, the guideline is "be consistent with pre-existing code unless you understand why the old code must be wrong.")


Uh...? This completely changes the meaning of e.g. _SecondType<int> from "ill-formed" to "well-formed __nat." There's no way that's safe.

philnik updated this revision to Diff 414960.Mar 13 2022, 2:21 PM
philnik marked 3 inline comments as done.
  • Rebased
  • Address comments
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 2:21 PM
philnik added inline comments.Mar 13 2022, 2:21 PM

I wasn't able to reproduce it.


_FirstType and _SecondType are exclusively used in <tuple> and only in is_* contexts. If I'm not mistaken __nat should be false in every single case.

EricWF added a subscriber: EricWF.Mar 18 2022, 10:28 AM

I would like to test this against Google's source. I'll try to get that done by early this week. Please do not commit this until then.
Tuple is so sensitive that changing whitespace breaks users.


What's __maybe_const? Should it be in this patch?


These bits of SFINAE were written very specifically because tuple is a very weird beast.

I'm surprised we don't have a test in the test suite that breaks after changing this, because I thought I added one.

There are certain SFINAE loops that crop up where checking these properties in the specific order is/was important.


What's with the reference here


__nat is an awful hack. Let's not user it more.


I'm with Arthur, this should not succeed. This needs to be substitution failure.

EricWF added inline comments.Mar 18 2022, 10:34 AM

Ignore this.

philnik updated this revision to Diff 416574.Mar 18 2022, 12:28 PM
philnik marked 3 inline comments as done.
  • Fixes

@EricWF This patch will probably not be ready within the next week. I can ping you again before landing this if you want.


In which case does it have to be a substitution failure? IIRC there are cases where I need it to not be a substitution failure.

philnik updated this revision to Diff 416687.Mar 19 2022, 4:49 AM
  • Try to fix CI
philnik updated this revision to Diff 416690.Mar 19 2022, 5:29 AM
  • Next try
philnik updated this revision to Diff 416948.Mar 21 2022, 8:06 AM
  • Fix no exceptions
philnik updated this revision to Diff 417045.Mar 21 2022, 12:10 PM
  • Add test_macros include
ldionne requested changes to this revision.Mar 24 2022, 1:29 PM

Thanks a lot for the patch! This is a tough one for sure. I thought we would never add new constructors to tuple, but it looks like they did it again!


I would *not* guard this with _LIBCPP_STD_VER since this is internal. We can guard the top-level swap on std::tuple only.


Since this is new code, I would recommend not using inline here.


Same, I would *not* guard this with _LIBCPP_STD_VER since this is internal.

ldionne added inline comments.Mar 24 2022, 1:29 PM

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.


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.


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.


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


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

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


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.

16 ↗(On Diff #417045)

Why does it fail on GCC?

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.


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.




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

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.


Are we implementing this for std::array too?

EricWF added inline comments.Mar 25 2022, 2:15 PM

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.


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


Use _If here to avoid adding more template instantiations.

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


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?


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

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

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!


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


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


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


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.


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.


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.


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


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


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


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


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


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.


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.


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




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


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.


This file is missing a license :)




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


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?


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


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.


I think you're right.


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


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




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!