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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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;
----------------
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.


================
Comment at: libcxx/test/support/test_iterators.h:758
+
+  constexpr friend void iter_swap(SwapCountingIterator& lhs, SwapCountingIterator& rhs) {
+    std::ranges::iter_swap(lhs.iter_, rhs.iter_);
----------------
huixie90 wrote:
> take the arguments by const ref, as `indirectly_swappable` requires const
Weird that this didn't show in the test. Maybe I just missed it.


================
Comment at: libcxx/test/support/test_iterators.h:760
+    std::ranges::iter_swap(lhs.iter_, rhs.iter_);
+    ++*lhs.counter_;
+  }
----------------
huixie90 wrote:
> why does the `rhs` counter not incremented?
Because that would increment the counter two times per swap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129859