[libcxx-commits] [PATCH] D102809: [libcxx][ranges] Add `ranges::iter_swap`.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 11 inline comments as done.
zoecarver added a comment.

@ldionne this is ready for review.

@cjdb this is ready for re-review.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:142
+  HasIterSwap a(value1), b(value2);
+  std::ranges::iter_swap(a, b);
+  assert(value1 == 1 && value2 == 1);
----------------
Quuxplusone wrote:
> This is the only place you test `std::ranges::iter_swap(lvalue, lvalue)`. Please expand the tests below to test them also with lvalues of type `HasRangesSwapWrapper`, `A*`, etc., at least enough to hit each different overload of `iter_swap` once with at least one lvalue.
> 
> You never test `std::ranges::iter_swap(lvalue, rvalue)` or `std::ranges::iter_swap(rvalue, lvalue)`, but I don't think those need testing, as long as you hit the `(lvalue, lvalue)` cases.
OK. Added a few more lvalue tests to cover the remaining two overloads.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/unqualified_lookup_wrapper.h:75
+
+  [[nodiscard]] constexpr int moves() const noexcept { return moves_; }
+
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > `int moves() const { return moves_; }` :)
> `moves()` still doesn't need `noexcept`.
I think it makes sense to leave the constexpr in this case. This is a sort of general utility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809