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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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;
----------------
var-const wrote:
> 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.
I think we should reuse. Teaching `reverse` about `_AlgPolicy` should be trivial.


================
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) {
----------------
philnik wrote:
> var-const wrote:
> > Is the output of `prev_permutation` deterministic? If yes, I'd prefer several simple test cases that check the exact output.
> Yes, the output is deterministic. This test checks the exact output of `prev_permutation`. It checks after every  call to `ranges::prev_permutation` that it's lexicographically less than the input. It's also checked that there were N! rounds. Since there are N! permutations of a range, every input to `prev_permutation` has to have resulted in the permutation before the input.
I think this is a clever way to test this. Maybe a little bit too clever to be the only test we have. I'd be OK with this approach if it were also augmented with a small number (e.g. 10 with boundary conditions) of explicit input/output comparisons. Does that sound reasonable?

Also, a comment explaining what the testing methodology here would be really helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129859