Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++] implement "pair" section of P2321R2 `zip`
ClosedPublic

Authored by huixie90 on Aug 9 2022, 6:56 AM.

Details

Summary
  • add constructors that takes non-const lvalue ref pair and const rvalue ref pair
  • add const assignment operators
  • add const swap member and non member

(note) the pair changes in uses_allocator_construction_args has not been implemented as the C++20 paper P0591r4 has not been implemented in libcxx

Diff Detail

Event Timeline

huixie90 created this revision.Aug 9 2022, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 6:56 AM
huixie90 updated this revision to Diff 451526.Aug 10 2022, 9:43 AM

implement swap

huixie90 published this revision for review.Aug 10 2022, 9:46 AM
huixie90 edited the summary of this revision. (Show Details)
huixie90 added reviewers: ldionne, philnik, var-const.
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 9:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const requested changes to this revision.Aug 23 2022, 3:15 PM

Thanks a lot for working on this! Sorry about the slow review. I'll take a look at the tests soon.

libcxx/include/__utility/pair.h
213

The noexcept doesn't seem to be in the standard -- we're adding it for consistency with existing constructors, right?

321

Same question here re. noexcept.

386

I think you don't have to use the macro and can use just plain noexcept since it won't be available in older language modes.

516

The noexcept specification is different from the standard -- looks like it's copied from the member version.

This revision now requires changes to proceed.Aug 23 2022, 3:15 PM
var-const added inline comments.Aug 23 2022, 5:45 PM
libcxx/include/utility
91

This looks incorrect -- the template arguments are U and V, but the function arguments refer to U1 and U2.

libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_const_copy_convert.pass.cpp
22

It looks like these types should be moved somewhere under test_support.

libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor_pair_U_V_const_move.pass.cpp
16

Nit: s/U2/U1/.

21

Similar to the other tuple include: these types should live somewhere under test_support.

44

Nit: can line breaks here be more consistent?

libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor_pair_U_V_ref.pass.cpp
16

Nit: s/U2/U1/.

libcxx/test/std/utilities/utility/pairs/pairs.pair/swap_member_const.pass.cpp
26

Should this be const T&?

42

Optional: instead of using mutable, you could store a pointer or a reference, then modify the pointed-to value. That should fix the GCC issue.

huixie90 marked an inline comment as done.Aug 24 2022, 6:46 AM
huixie90 added inline comments.
libcxx/include/__utility/pair.h
213

that is right. None of the overload in the standard has noexcept and we added noexcept to the other overloads. I am just trying to be consistent with what we did in the past. Do you think it is reasonable?

321

that is right. None of the overload in the standard has noexcept and we added noexcept to the other overloads. I am just trying to be consistent with what we did in the past. Do you think it is reasonable?

516

The standard is noexcept(noexcept(x.swap(y))); which is equivalent to the one here. The reason I wrote this way is to be consistent with what we did in the past (see the overload above this one). do you think we should change it?

libcxx/include/utility
91

good catch

libcxx/test/std/utilities/utility/pairs/pairs.pair/swap_member_const.pass.cpp
26

could be but I prefer this way. here t1 and t2 are lvalues of T and T could be non const or const. In my usage in this test, I choose T to be const. see line 35 - 38. What do you think? If I make it const T& here, I need to call this concept ConstMemberSwapNoexcept`.

huixie90 updated this revision to Diff 455339.Aug 24 2022, 1:08 PM
huixie90 marked 8 inline comments as done.

address issues

libcxx/test/std/utilities/utility/pairs/pairs.pair/swap_member_const.pass.cpp
42

I think it might be better to have a test to test this value semantics type, since we have a test that tests pair of references for the reference semantics already. what do you think?

huixie90 updated this revision to Diff 455563.Aug 25 2022, 6:18 AM

attempt to fix CI

var-const accepted this revision as: var-const.Aug 25 2022, 10:29 AM

Thanks a lot for working on this! LGTM, but I'll leave the final approval to @ldionne.

libcxx/include/__utility/pair.h
213

I think it's the right thing to do, but please double-check with @ldionne.

516

I'd rather use the same specification as the standard unless there's a reason to diverge (it saves the mental burden of thinking whether our specification is equivalent).

ldionne accepted this revision.Sep 27 2022, 2:28 PM
This revision is now accepted and ready to land.Sep 27 2022, 2:28 PM
This revision was automatically updated to reflect the committed changes.