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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__iterator/iter_swap.h:42
+    indirectly_readable<_T1> && indirectly_readable<_T2> &&
+    swappable_with<decltype(*declval<_T1>()), decltype(*declval<_T2>())>;
+
----------------
I think it would be more in-line with what we usually do to say `swappable_with<iter_reference_t<_T1>, iter_reference_t<_T2>>` instead. WDYT?

As written, I think this is slightly incorrect (but I'm not sure it's possible to notice the difference). The Standard talks about the "reference type of `E1` and `E2`", which is `*declval<_T1&&>()`, not `*declval<_T1>()`. But regardless, I think we can just switch to `iter_reference_t<_T1>` unless there's a problem with that I'm not seeing.


================
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>>()))
----------------
I don't see how to do better, but it's awful to have to duplicate the body like that.


================
Comment at: libcxx/include/__iterator/iter_swap.h:55-56
+  struct __fn {
+    template <class _T1, class _T2>
+      requires __unqualified_iter_swap<_T1, _T2>
+    constexpr void operator()(_T1&& __x, _T2&& __y) const
----------------
zoecarver wrote:
> cjdb wrote:
> > 
> This is a bit more confusing imho. If you feel strongly I'll change it, though. 
Agreed, I would definitely keep as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809