Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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). |
libcxx/include/__iterator/reverse_iterator.h | ||
---|---|---|
327 | inline is unnecessary since this is a template. | |
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. |
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. |
- 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. |