Page MenuHomePhabricator

[libc++][ranges] Implement `ranges::shuffle`.
ClosedPublic

Authored by var-const on Jul 21 2022, 6:09 PM.

Details

Diff Detail

Event Timeline

var-const created this revision.Jul 21 2022, 6:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 6:09 PM
var-const requested review of this revision.Jul 21 2022, 6:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 6:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 446675.Jul 21 2022, 6:15 PM

Add a comment, update patch number.

var-const updated this revision to Diff 446678.Jul 21 2022, 6:20 PM

Mark a libc++-specific check as such.

var-const updated this revision to Diff 446682.Jul 21 2022, 6:26 PM

Improve comments, fix the CI.

var-const added inline comments.Jul 21 2022, 6:30 PM
libcxx/test/std/algorithms/alg.modifying.operations/alg.random.shuffle/ranges_shuffle.pass.cpp
243

The standard requirement that the number of swaps is "exact" instead of "at most" doesn't make sense to me. Is this a wording issue, or am I missing something?

Add forgotten hide-from-ABI annotations, undefine min/max macros.

var-const updated this revision to Diff 446743.Jul 22 2022, 1:47 AM

Fix the CI.

ldionne accepted this revision.Jul 22 2022, 6:17 AM
ldionne added inline comments.
libcxx/test/std/algorithms/alg.modifying.operations/alg.random.shuffle/ranges_shuffle.pass.cpp
243

It is somewhat unusual indeed -- I don't think anybody would complain for making fewer swaps than specified.

This revision is now accepted and ready to land.Jul 22 2022, 6:17 AM
This revision was automatically updated to reflect the committed changes.
jloser added a subscriber: jloser.Jul 22 2022, 10:26 AM
jloser added inline comments.
libcxx/test/std/algorithms/alg.modifying.operations/alg.random.shuffle/ranges_shuffle.pass.cpp
243

There are some algorithms that enforce exactly a certain number of swaps (like swap_ranges), but I think the wording should be changed to say "at most" rather than "exactly" for ranges::shuffle. I filed https://github.com/cplusplus/draft/issues/5636 for clarification.

LGTM

libcxx/test/support/test_iterators.h
828

not sure how this is used in the test but usually iter_swap would swap the underlying references i.e std::swap(*a.ptr_, *b.ptr_)

huixie90 added inline comments.Jul 22 2022, 11:09 AM
libcxx/include/__algorithm/ranges_shuffle.h
70

not an issue but just write it down as I had thought about it.

Not sure how this could possibly go wrong but the original requirement is that you can do std::invoke(__gen). I guess it is ok to call __gen() instead of but I'd still prefer to stick to original requirement