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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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);
----------------
var-const wrote:
> Question: why is `static_assert` here instead of the common pattern of asserting `test()` in `main()`?
The `static_assert`s are here because putting them anywhere else would hit the constant evaluation step limit.


================
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); };
+
----------------
var-const wrote:
> Does the argument have to be forwarded, perhaps? (here and in the other concept above)
I don't think so, since `{prev, next}_permutation` only supports copyable iterators/ranges.


================
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) {
----------------
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.


================
Comment at: libcxx/test/support/test_iterators.h:755
+  int* counter_;
+  SwapCountingIterator() = default;
+  constexpr SwapCountingIterator(const Iter& iter, int* counter) : iter_(iter), counter_(counter) {}
----------------
var-const wrote:
> Nit: this creates an object with uninitialized variables (`counter_` and potentially `iter_`), maybe value-initialize those?
These are uninitialized on purpose. Algorithms aren't allowed to read from a default-constructed allocator and the tests always have to provide one. If these are uninitialized for any reason the constexpr tests will fail. That wouldn't be the case if I value-initialized them.


================
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_);
----------------
var-const wrote:
> Would it be possible to just inherit from `Iter` and add the constructor and the `iter_swap` specialization in the derived class?
Inheriting wouldn't work with raw pointers. (inheriting was actually my initial design)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129859