Page MenuHomePhabricator

[libcxx] adds std::ranges::swap, std::swappable, and std::swappable_with
ClosedPublic

Authored by cjdb on Feb 21 2021, 1:29 PM.

Details

Summary

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Depends on D96742

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 21 2021, 1:29 PM
cjdb added a comment.Feb 21 2021, 1:31 PM

I'm committing swappable independently from swappable_with so that I can get movable, copyable, semiregular and regular out first (all of which are arguably more important than swappable_with).

cjdb updated this revision to Diff 325367.Feb 21 2021, 8:17 PM

fixes lambdas that weren't called

I find the tests really hard to read and follow. I think giving the lambdas an explicit name would be really beneficial.

libcxx/include/concepts
268

I think this is already covered by the explicit _Size parameter

296

I *think* this should not be done. We would lose a tone of throughput due to the additional checking done inside ranges::swap_ranges.

Is there a measurable benefit in using ranges::swap_ranges

307

While this is what the standard says, it boils down to the conventional swap implementation, which is better known. So should we simply avoid the function call and directly open code swap?

312

This *think* this must be inside another inline namespace _meow I tend to happily forget the exact reason

In general I would say we should put the whole CPO into the ranges namespace and then use an inline namespace here. It is a bit strange that the CPO itself lives outside of ranges

libcxx/test/std/concepts/lang/swappable.pass.cpp
39–40

I find this supper hard to read I think we should enforce some linebreaks with //

68

Please wrap this into a separate function with a descriptive name

cjdb updated this revision to Diff 326590.Feb 25 2021, 8:38 PM

rebases to activate CI

cjdb updated this revision to Diff 327971.Mar 3 2021, 5:34 PM
cjdb marked an inline comment as done.
  • applies @miscco's feedback
  • adds std::swappable_with

Not a real review, just a couple drive-by comments. I'm unlikely to do a real review. :)

libcxx/include/concepts
307

IMO yes; and also std::move should be _VSTD::move.

312

One thing that might be affected by this comment, and should certainly be tested, is whether swap itself is swappable:

decltype(std::ranges::swap) s1, s2;
std::ranges::swap(s1, s2);

I don't know whether it should be or not, but ISTR that this was the trouble spot for range-v3 swap a couple years ago.

cjdb marked 2 inline comments as done.Mar 3 2021, 6:49 PM
cjdb added inline comments.
libcxx/include/concepts
268

Nice catch :-)

296

http://eel.is/c++draft/concept.swappable#2.2 explicitly mentions it as the call, but if we can skip the call, I'm all for it.

307

I'd prefer to faithfully follow the standard and trust an optimising compiler will deal with this appropriately :-)

312

The type lives in namespace std::ranges::__swap. I don't see any wording in [[customization.point.object]][1] or [[concepts.swappable]][2] that mandates this, though I do recall that range-v3 (and possibly cmcstl2) needed to do this in C++14 mode since there weren't inline variables at the time.

[1]: http://eel.is/c++draft/customization.point.object
[2]: http://eel.is/c++draft/concepts.swappable

libcxx/test/std/concepts/lang/swappable.pass.cpp
39–40

clang-format, why you do dis?

tcanens added a subscriber: tcanens.Mar 3 2021, 7:17 PM
tcanens added inline comments.
libcxx/include/concepts
312

[concepts.syn] mandates it.

This is required because otherwise you can't declare hidden friend swaps for anything in namespace ranges; the same namespace member can't be both a variable and a function, even if the function is otherwise invisible to ordinary lookup.

cjdb updated this revision to Diff 327996.Mar 3 2021, 7:45 PM
cjdb marked an inline comment as done.

puts ranges::swap in an inline sub-namespace

cjdb updated this revision to Diff 328001.Mar 3 2021, 7:54 PM

adds test to see if ranges::swap is swappable

cjdb added inline comments.Mar 3 2021, 7:55 PM
libcxx/include/concepts
312

Yep, makes sense, done.

312

Test added.

cjdb marked 4 inline comments as done.Mar 3 2021, 7:56 PM
cjdb updated this revision to Diff 328032.Mar 3 2021, 10:51 PM
cjdb retitled this revision from [libcxx] adds std::ranges::swap and std::swappable to [libcxx] adds std::ranges::swap, std::swappable, and std::swappable_with.

s/std::move/_VSTD::move/

cjdb added inline comments.Mar 3 2021, 10:55 PM
libcxx/include/concepts
307

_VSTD::move done, but I'd still prefer to follow the standard wording.

miscco accepted this revision.Mar 4 2021, 12:56 AM

Not a maintainer, but this looks good to me

libcxx/include/concepts
312

Note, that this does not really make sense, as the standard requires, that there is only a single instance of each CPO, but It definitly does not hurt

cjdb added inline comments.Mar 4 2021, 10:34 AM
libcxx/include/concepts
312

as the standard requires, that there is only a single instance of each CPO

Where does it say that? [customization.point.object]/3 says that all instances must be equal, but that doesn't preclude having a two of the same CPO (not being allowed to copy would make passing CPOs to algorithms illegal).

It's still a good idea to test, since we need to check that the CPO satisfies semiregular.

EricWF accepted this revision as: EricWF.Mar 4 2021, 2:19 PM
EricWF added inline comments.
libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
565

Why are these commented out?

EricWF accepted this revision.Mar 4 2021, 2:26 PM

LGTM assuming the inline comments get addressed.

libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

Can we write these tests in a way that makes them non-threading dependent.

This revision is now accepted and ready to land.Mar 4 2021, 2:26 PM
ldionne requested changes to this revision.Mar 4 2021, 2:46 PM
ldionne added inline comments.
libcxx/include/concepts
296

I don't understand Michael's comment about the loss of throughput for using swap_ranges here. It seems to me like that would be the correct way of implementing this. Why reinvent the wheel?

309

// end namespace ranges::__swap

libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

I don't think so since he's explicitly trying to exercise these types, which aren't provided when _LIBCPP_HAS_NO_THREADS. Better would be to have a test-suite version of the macro (to express the fact that the test suite supports being used when threads are not available), but that's for a different patch.

This revision now requires changes to proceed.Mar 4 2021, 2:46 PM
ldionne accepted this revision.Mar 4 2021, 3:04 PM
ldionne added inline comments.
libcxx/include/concepts
296

Thinking further, obviously we can't use ranges::swap_ranges before it has been implemented. I think we should add a TODO and come fix it later once it's implemented so that we are in line with the spec.

This revision is now accepted and ready to land.Mar 4 2021, 3:04 PM
cjdb updated this revision to Diff 328341.Mar 4 2021, 5:13 PM
cjdb marked 2 inline comments as done.

adds union to definition (and tests), uncomments tests, adds TODO

cjdb marked 2 inline comments as done.Mar 4 2021, 5:14 PM

Will commit tomorrow.

libcxx/include/concepts
309

Also for the namespace below.

libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
565

Uncommented.

594

How about I do that patch just before announcing P0898 is implemented?

cjdb updated this revision to Diff 328358.Mar 4 2021, 6:47 PM

fixes union swap

Quuxplusone added inline comments.Mar 4 2021, 7:27 PM
libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

Peanut gallery says: Other alternatives include

  • test a made-up struct Immobile instead of specifically std::mutex; we can assume that std::mutex is equally immobile
  • move tests of std::mutex's own properties into test/std/thread/thread.mutex/ (because if someone were somehow to break the immobility of std::mutex, I'd hope that it's the mutex tests that fail, not some random concept's tests)

But I have no particular stake in the outcome.

cjdb added inline comments.Mar 4 2021, 7:32 PM
libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

Reason for having std::mutex over immobile is because @ldionne asked if I could test some standard types too, and I couldn't think of any non-thread based nonmovable types at the time. @EricWF and I agreed to change the mutex ones to some streams.

cjdb updated this revision to Diff 328382.Mar 4 2021, 9:40 PM

replaces std::mutex and std::lock_guard with std::ios_base and std::basic_ios

cjdb updated this revision to Diff 328572.Mar 5 2021, 10:09 AM

CI sanity check before submitting (hopefully a flake)

ldionne added inline comments.Mar 5 2021, 11:40 AM
libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

I think you'll have the same problem with _LIBCPP_HAS_NO_LOCALIZATION now, unless I'm mistaken. You could move the test to the mutex tests instead if you wanted. Or remove those - they do provide nice testing coverage but they're not worth creating a ruckus over IMO!

cjdb added inline comments.Mar 5 2021, 12:08 PM
libcxx/test/std/concepts/lang/swappable_with.compile.pass.cpp
594

Okay, so that's why CI failed this time, thanks for the clarity there :-)

In that case, I'll take Arthur's suggestion and make an immobile type, and in a later patch, make a test for thread and stream types.

cjdb updated this revision to Diff 328624.Mar 5 2021, 1:03 PM

replaces immovable types