[libcxx-commits] [PATCH] D129859: [libc++] Implement ranges::{prev, next}_permutation

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/CMakeLists.txt:123
   __algorithm/ranges_push_heap.h
+  __algorithm/ranges_prev_permutation.h
   __algorithm/ranges_remove.h
----------------
Looks like this line is unsorted (might be what the CI is complaining about).


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:45
+    return {std::move(__last), false};
+  while (true) {
+    auto __ip1 = __i;
----------------
Nit: can you add a blank line before this line?


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:48
+    --__i;
+    if (std::invoke(__comp, std::invoke(__proj, *__ip1), std::invoke(__proj, *__i))) {
+      auto __j = __last;
----------------
Nit: blank line above.


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:53
+      } while (!std::invoke(__comp, std::invoke(__proj, *__j), std::invoke(__proj, *__i)));
+      ranges::iter_swap(__i, __j);
+      ranges::reverse(__ip1, __last);
----------------
Nit: blank line above.


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:57
+    }
+    if (__i == __first) {
+      ranges::reverse(__first, __last);
----------------
Nit: blank line above.


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:71
+  operator()(_Iter __first, _Sent __last, _Comp __comp = {}, _Proj __proj = {}) const {
+    return ranges::__prev_permutation_impl(__first, __last, __comp, __proj);
+  }
----------------
Nit: move?


================
Comment at: libcxx/include/__algorithm/ranges_prev_permutation.h:40-62
+__prev_permutation_impl(_Iter __first, _Sent __sent, _Comp& __comp, _Proj& __proj) {
+  auto __last = ranges::next(__first, __sent);
+  auto __i = __last;
+  if (__first == __last || __first == --__i)
+    return {__last, false};
+  while (true) {
+    auto __ip1 = __i;
----------------
philnik wrote:
> huixie90 wrote:
> > I think this algorithm is complex enough to reuse the classic one. what do you think?
> I think it's harder to combine the two than to use separate ones. The algorithms isn't //that// long, and I would have to teach `reverse` to dispatch between versions. If that changes we can still think about combining the two.
I'd also prefer reuse, but don't want to block this patch on this.


================
Comment at: libcxx/include/algorithm:736
+    requires sortable<I, Comp, Proj>
+    constexpr ranges::prev_permutation_result<I>
+      ranges::prev_permutation(I first, S last, Comp comp = {}, Proj proj = {});                    // since C++20
----------------
Please also add the definition of the result types to the synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.permutation.generators/ranges.next_permutation.pass.cpp:121
+  test_prev_permutations(iterator_overload);
+  static_assert(test_prev_permutations(iterator_overload));
+  test_prev_permutations(range_overload);
----------------
Question: why is `static_assert` here instead of the common pattern of asserting `test()` in `main()`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.permutation.generators/ranges.next_permutation.pass.cpp:45
+template <class Range>
+concept HasNextPermutationR = requires(Range range) { std::ranges::next_permutation(range); };
+
----------------
Does the argument have to be forwarded, perhaps? (here and in the other concept above)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.permutation.generators/ranges.next_permutation.pass.cpp:63
+void test_permutations() {
+  auto test_prev_permutations = [](auto call_prev_permutation) {
+    auto input = std::array{1, 2, 3, 4, 5, 6};
----------------
There's a bunch of references to `prev_permutation` here. I presume this is simply a copy-pasting issue? (if there's a reason to test them here, please point it out)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.permutation.generators/ranges.next_permutation.pass.cpp:140
+
+int main(int, char**) { test(); }
----------------
`return 0;` (unless something changed recently in that regard)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.permutation.generators/ranges.prev_permutation.pass.cpp:91
+
+  auto iterator_overload = [](auto&& first, auto&& last) { return std::ranges::prev_permutation(first, last); };
+  auto range_overload    = [](auto&& first, auto&& last) {
----------------
Is the output of `prev_permutation` deterministic? If yes, I'd prefer several simple test cases that check the exact output.


================
Comment at: libcxx/test/support/test_iterators.h:755
+  int* counter_;
+  SwapCountingIterator() = default;
+  constexpr SwapCountingIterator(const Iter& iter, int* counter) : iter_(iter), counter_(counter) {}
----------------
Nit: this creates an object with uninitialized variables (`counter_` and potentially `iter_`), maybe value-initialize those?


================
Comment at: libcxx/test/support/test_iterators.h:758
+
+  constexpr friend void iter_swap(const SwapCountingIterator& lhs, const SwapCountingIterator& rhs) {
+    std::ranges::iter_swap(lhs.iter_, rhs.iter_);
----------------
Would it be possible to just inherit from `Iter` and add the constructor and the `iter_swap` specialization in the derived class?


================
Comment at: libcxx/test/support/test_iterators.h:763
+
+  constexpr SwapCountingIterator& operator++() {
+    ++iter_;
----------------
(I wish we had something like `Boost.Iterator` to cut down on this boilerplate. Of course, it's not related to this patch)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129859/new/

https://reviews.llvm.org/D129859