Page MenuHomePhabricator

[libc++] Implement ranges::{reverse, rotate}_copy
Needs ReviewPublic

Authored by philnik on Jun 7 2022, 6:08 AM.

Details

Reviewers
ldionne
Mordante
var-const
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

philnik created this revision.Jun 7 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 6:08 AM
Herald added a subscriber: mgorny. · View Herald Transcript
philnik requested review of this revision.Jun 7 2022, 6:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 6:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const requested changes to this revision.Thu, Jun 9, 6:17 PM
var-const added inline comments.
libcxx/docs/Status/RangesAlgorithms.csv
56–58

As per usual, please update. :)

libcxx/include/__algorithm/ranges_reverse_copy.h
20

Nit: include <__utility/move.h>.

26

Please add the language version / no incomplete ranges guard (other file as well).

40–41

These functions are verbose and using them seems a little clunky (they take a different number of arguments, it's not obvious whether e.g. begin in the name means "make rbegin" or "make rend from the begin iterator", etc.). How about having a single function that returns a pair of iterators instead? I don't think it should add any noticeable overhead:

auto [__rfirst, __rlast] = std::__to_reverse_iterators(__first, __last);
auto __ret = ranges::copy(std::move(__rfirst), std::move(__rlast), std::move(__result));
libcxx/include/algorithm
1294–1295

Please add the synopsis.

libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp
13

Missing synopsis.

48

Nit: decltype(auto).

libcxx/test/std/algorithms/alg.modifying.operations/alg.rotate/ranges.rotate_copy.pass.cpp
13

Missing synopsis.

24

s/Reverse/Rotate/.

48

Nit: decltype(auto).

76

Please also check with a two-element range (the smallest range that can actually be rotated, and it has an even number of elements which otherwise is not tested).

This revision now requires changes to proceed.Thu, Jun 9, 6:17 PM
philnik updated this revision to Diff 438588.Tue, Jun 21, 1:15 AM
philnik marked 11 inline comments as done.
  • Address comments
ldionne added inline comments.Thu, Jun 30, 6:11 AM
libcxx/include/__iterator/reverse_iterator.h
327

inline is unnecessary since this is a template.
I suspect constexpr won't work in C++11/14/17 (but you could check 17, it might).

libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp
30

I would like an explicit test that is_same<reverse_copy_result<....>, in_out_result<...>>. In fact, I don't see that type mentioned anywhere in the tests, so we wouldn't even notice if we didn't define it. Please do that here but also for all the other algorithms that have been added and that had accompanying foo_result types.

var-const accepted this revision as: var-const.Fri, Jul 1, 4:49 PM

LGTM with just a couple of minor things. I'll leave the final approval to @ldionne since he had comments.

libcxx/include/__iterator/reverse_iterator.h
337

make_pair?

libcxx/include/algorithm
429

Please add the definition of {reverse,rotate}_copy_result to the synopsis as well.

libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp
30

Maybe create a dedicated file for this, similar to ranges_robust_against_copying_*? It seems like these checks will be really similar.

philnik updated this revision to Diff 442504.Wed, Jul 6, 3:52 AM
philnik marked 5 inline comments as done.
  • Address comments

@var-const could you take another look? I basically rewrote ranges::reverse_copy.

libcxx/test/std/algorithms/alg.modifying.operations/alg.reverse/ranges.reverse_copy.pass.cpp
30

I'll make a dedicated ranges_result_alias_declarations.compile.pass.cpp in it's own PR.