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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 15 inline comments as done.
zoecarver added inline comments.


================
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>())>;
----------------
cjdb wrote:
> 
In other PRs I've been asked to put the `!__unqualified_iter_swap` part in the requires clause. I also like this better because then the concept is more composable (rather than only being useful in one place). 


================
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
----------------
cjdb wrote:
> 
This is a bit more confusing imho. If you feel strongly I'll change it, though. 


================
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>
----------------
zoecarver wrote:
> 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. 
I'm going to leave this and mark it as resolved. 


================
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
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > 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? 
> > @tcanens said "the type of an expression is never a reference type," so no, you'd have to remove the reference first (e.g. if `_T1` is `int&`, then we should apply the trait to `int`, not to `int&`).
> I'm not sure I have the correct words to describe what I'm thinking here but won't the `&&` in `_T1&&` "eat" the reference part of the reference type? 
> I'm not sure I have the correct words to describe what I'm thinking here but won't the && in _T1&& "eat" the reference part of the reference type?

@Quuxplusone ping, does this make any sense?


================
Comment at: libcxx/include/__iterator/iter_swap.h:80
+    {
+      (void)(*_VSTD::forward<_T1>(__x) = __iter_exchange_move(_VSTD::forward<_T2>(__y), _VSTD::forward<_T1>(__x)));
+    }
----------------
Quuxplusone wrote:
> Here, `__iter_exchange_move` needs to be ADL-proofed — but actually, I strongly suggest that you just inline its body right here (and inline its noexcept-specification on line 78). Yes I know the Standard factors it out; but it's used only once and the factoring-out doesn't seem to do anything but make the code longer (both in the Standard and in libc++).
That's not a bad idea, I have to add a move for `__old`, but I think that's alright. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:37-41
+  constexpr friend void iter_swap(HasIterSwap& a, bool& b) {
+    auto tmp = a.value;
+    a.value = b;
+    b = tmp;
+  }
----------------
Quuxplusone wrote:
> Here and line 54: I'm worried that your implementation of this function is unnecessary and/or indistinguishable from the default `ranges::swap`, which means you're not actually testing that it's called. How about replacing these implementations with
> ```
> friend constexpr void iter_swap(HasIterSwap& a, HasIterSwap& b) {
>     called1 = true;
> }
> friend constexpr void iter_swap(HasIterSwap& a, bool& b) {
>     called2 = true;
> }
> ```
> and then writing tests that `assert(called1 && !called2)` (or whatever you expect to happen).
> For constexpr-friendliness, you'll probably need to make your constructor `explicit HasIterSwap(bool& c1, bool& c2) : called1(c1), called2(c2) {}`. Slack me if this doesn't make sense.
Updated to have something similar to this. Let me know what you think. 

For the record: this isn't really meant to test that we use the correct overload, though. I was just testing that it calls "the overload" it didn't really cross my mind that there would be any way for us to even call the `bool&` overload with an argument of type `HasIterSwap`. Anyway, I think it is a good idea to not have this just behave like an assignment operator or default swap impl. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:84
+
+// TODO: is there any way to observe the number of evaluations (p4.3)?
+
----------------
Quuxplusone wrote:
> Yes — `ranges::iter_swap(++i, ++j)` is supposed to increment `i` only once, not twice.
> But this is physically obvious. The Standard needs to mention it because the Standard is pretending that these are expressions-not-function-calls. We don't have to test it; you can remove this TODO.
Interesting, OK, thanks for explaining. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809