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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added a comment.

Some initial feedback, will give the tests a more in-depth review in a day or so.



================
Comment at: libcxx/include/__iterator/iter_swap.h:39-42
+  template<class _T1, class _T2>
+  concept __readable_swappable =
+    indirectly_readable<_T1> && indirectly_readable<_T2> &&
+    swappable_with<decltype(*declval<_T1>()), decltype(*declval<_T2>())>;
----------------



================
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
----------------



================
Comment at: libcxx/include/__iterator/iter_swap.h:60
+    {
+      (void)iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
+    }
----------------
tcanens wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > Here and line 80, I suggest removing the cast to `(void)`. IIUC, the reason it's there in https://eel.is/c++draft/iterator.cust.swap#4.1 is to make the "expression-equivalent-to" part work out to the correct type.
> > > However, it does occur to me that a cast to `(void)` is technically //not// a no-op, because it affects the well-formedness of the expression when `iter_swap` has been marked `[[nodiscard]]` (and a user's ADL `iter_swap` //might// be marked `[[nodiscard]]` for all we know).
> > > I'd be interested in @tcanens' take on whether the `(void)` cast is indispensable.
> > I'm slightly against removing the void cast, basically to make sure a case like `ensureVoidCast` works below. 
> nodiscard has zero normative effect, so it can't affect the well-formedness of anything.
I would prefer it if `ranges::iter_swap` didn't randomly cause people to want to enable `-Wno-unused-result`. Let's keep the cast to void.


================
Comment at: libcxx/include/__iterator/iter_swap.h:63-65
+    template <class _T1, class _T2>
+      requires (!__unqualified_iter_swap<_T1, _T2>) &&
+               __readable_swappable<_T1, _T2>
----------------
I'll leave the last overload as an exercise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809