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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with my comments applied.



================
Comment at: libcxx/include/__iterator/iter_swap.h:68-70
+      noexcept(noexcept(iter_value_t<_T2>(ranges::iter_move(__y))) &&
+               noexcept(*__y = ranges::iter_move(__x)) &&
+               noexcept(*_VSTD::forward<_T1>(__x) = declval<iter_value_t<_T2>>()))
----------------
zoecarver wrote:
> ldionne wrote:
> > I don't see how to do better, but it's awful to have to duplicate the body like that.
> I agree. We should really have a `noexcept(auto)`. This wouldn't be too hard to add to clang, and then we could just let GCC users have worse codegen...
If only that's how things worked 😅


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:26
+
+static_assert(std::semiregular<std::remove_cvref_t<IterSwapT&>>);
+
----------------
Based on http://eel.is/c++draft/customization.point.object#2, I would just do `std::semiregular<std::remove_cv_t<IterSwapT>>`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:138
+
+int main(int, char**) {
+  int value1 = 0;
----------------
Minor thing: I like to split test different test cases by enclosing them in a different scope. It makes it easier to see that they are different test cases and that they don't share anything between themselves. So I'd do:

```
{
    int value1 = 0;
    int value2 = 0;
    HasIterSwap a(value1), b(value2);
    std::ranges::iter_swap(a, b);
    assert(value1 == 1 && value2 == 1);
}
{
    int value1 = 0;
    int value2 = 0;
    HasRangesSwap c(value1), d(value2);
    HasRangesSwapWrapper cWrapper(c), dWrapper(d);
    std::ranges::iter_swap(cWrapper, dWrapper);
    assert(value1 == 1 && value2 == 1);
}

// etc...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809