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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__iterator/iter_swap.h:60
+    {
+      (void)iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
+    }
----------------
cjdb wrote:
> 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.
> 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.

+1 I'm going to leave it. (I don't see any way it hurts to have it, but I can see ways it helps.)


================
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>
----------------
cjdb wrote:
> I'll leave the last overload as an exercise.
When I've done this in other places, I've been asked to change it to use `requires`, and in this case (and especially below) I think it might be clearer than trying to fit it into the template. 


================
Comment at: libcxx/include/__iterator/iter_swap.h:75-76
+                !__readable_swappable<_T1, _T2>) &&
+               indirectly_movable_storable<_T1, _T2> &&
+               indirectly_movable_storable<_T2, _T1>
+    constexpr void operator()(_T1&& __x, _T2&& __y) const
----------------
Quuxplusone wrote:
> tcanens wrote:
> > zoecarver wrote:
> > > Quuxplusone wrote:
> > > > This should say `indirectly_movable_storable<_T1&&, _T2&&>`, right? because the type of `E1` is not `_T1` but rather `_T1&&`?
> > > > Can you find a test case that demonstrates the difference, and add it as a regression test?
> > > > This comment //probably// applies equally to all your uses of exposition-only helpers such as `__readable_swappable`, but I'm even less confident that that difference will be detectable by a test.
> > > This is my bad for naming these `_T1` and `_T2` so when I read the standard wording, my brain thought it meant to just use the template params (which don't exist in the wording). I think you're right. 
> > The type of an expression is never a reference type.
> > The type of an expression is never a reference type.
> 
> So all of these trait checks should be using e.g. `indirectly_movable_storable<remove_reference_t<_T1>, remove_reference_t<_T2>>`? Blech. OK.
> So all of these trait checks should be using e.g. indirectly_movable_storable<remove_reference_t<_T1>, remove_reference_t<_T2>>? Blech. OK.

I think they can actually just be `_T1` and `_T2` (no `remove_reference_t`), right? 


================
Comment at: libcxx/include/__iterator/iter_swap.h:78
+    constexpr void operator()(_T1&& __x, _T2&& __y) const
+      noexcept(noexcept(__iter_exchange_move(_VSTD::forward<_T2>(__y), _VSTD::forward<_T1>(__x))))
+    {
----------------
tcanens wrote:
> This is missing the assignment,
You're right. I thought `__iter_exchange_move` would take care of this, but it only checks one of the assignment operators, not the other. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:155
+  std::ranges::iter_swap(arr.begin(), arr.begin() + 1);
+  assert(arr[0].moves() == 1 && arr[1].moves() == 2);
+
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > It seems like this is testing //the order in which// `std::swap` performs its moves — is that right? If so, I don't think it belongs in libcxx/test/std/ at all.  Ditto throughout.
> > Hmm, I see what you're saying with L155 specifically. And we probably don't really need this test (or maybe a better test would be `arr[0].moves() + arr[1].moves() == 3`. 
> > 
> > > Ditto throughout.
> > 
> > Where else do you see this?
> At least lines 159, 162, 165, 168, 171, 174, 177.
> At least lines 159, 162, 165, 168, 171, 174, 177.

These are just checking that the values of the array `buff` are swapped. I think this is exactly what we want to be checking. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809