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

Wed May 19 15:19:36 PDT 2021

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: tcanens.
Quuxplusone added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/include/__iterator/iter_swap.h:9-10
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___ITERATOR_SWAP_H
+#define _LIBCPP___ITERATOR_SWAP_H
+
----------------
`s/SWAP/ITER_SWAP/g`

================
Comment at: libcxx/include/__iterator/iter_swap.h:13-14
+#include <__config>
+#include <__iterator/iterator_traits.h>
+#include <__iterator/concepts.h>
+#include <__ranges/access.h>
----------------
alphabetize plz

================
Comment at: libcxx/include/__iterator/iter_swap.h:44-52
+  template<class _T1, class _T2>
+  constexpr iter_value_t<_T1> __iter_exchange_move(_T1&& __x, _T2 __y)
+    noexcept(noexcept(iter_value_t<_T1>(iter_move(__x))) &&
+             noexcept(*__x = iter_move(__y)))
+  {
+    iter_value_t<_T1> __old(iter_move(__x));
+    *__x = iter_move(__y);
----------------
Do we have any sense of whether the Standard intends this unqualified `iter_move` to refer to `std::ranges::iter_move` (which is what it currently refers to in this PR, IIUC) or to an ADL `iter_move` (which is what would make more sense IMHO IIUC)?
If it is intended to refer to `std::ranges::iter_move`, then please (for the love of god) replace `iter_move` with `ranges::iter_move` throughout.
If it is intended to be an ADL `iter_move`, then... yuck, but let's figure something out.

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

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

================
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)));
+    }
----------------
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++).

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:24
+
+using IterSwapT = decltype(std::ranges::iter_swap)&;
+
----------------
Has this been done in other tests already? If I'd seen it, I'd say why are you adding the `&`.
I would make `IterSwapT` an alias for `decltype(std::ranges::iter_swap)`, and then I would check //both// `std::invocable_v<IterSwapT, blah>` //and// a few `std::invocable_v<IterSwapT&, blah>` and `std::invocable_v<IterSwapT&&, blah>`. (Remember reference-collapsing means that with your current definition, all three of `IterSwapT{,&,&&}` are equivalent to `IterSwapT&`, and that strikes me as less-useful-than-it-could-be.)

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:30
+  bool value;
+  HasIterSwap(bool value) : value(value) {}
+
----------------
nit: `explicit` plz

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:32
+
+  constexpr friend void iter_swap(HasIterSwap& a, HasIterSwap& b) {
+    bool tmp = a.value;
----------------
https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/#friendness-to-befriend-an-entity :)

================
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;
+  }
----------------
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.

================
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)?
+
----------------
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.

================
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);
+
----------------
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.

================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/unqualified_lookup_wrapper.h:75
+
+  [[nodiscard]] constexpr int moves() const noexcept { return moves_; }
+
----------------
`int moves() const { return moves_; }` :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809