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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone resigned from this revision.
Quuxplusone added a comment.

I'm unlikely to have any further comments on this, but also don't think I should count as "accepting" it, since I've mainly done superficial stuff and @tcanens had some pretty significant standardese issues (which have probably been dealt with now but I'm not claiming I'm sure of that). I'll let someone-else be the final approver.



================
Comment at: libcxx/include/__iterator/iter_swap.h:74
+      *__y = ranges::iter_move(__x);
+      (void)(*_VSTD::forward<_T1>(__x) = _VSTD::move(__old));
+    }
----------------
Just `*_VSTD::forward<_T1>(__x) = _VSTD::move(__old);` — no need to cast to `void` here.


================
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:
> 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?
No; reference collapsing means that when `T` is `int&`, `T&&` is `int&` (not `int&&`).
However, at least for `iter_value_t`, the concept itself will strip cvref-qualifiers:
https://godbolt.org/z/aEEoen53P
and then for `indirectly_movable_storable`, I think it'd be difficult (and presumably library-UB, even if it were possible) to come up with a type where `indirectly_movable_storable<T>` was true and `indirectly_movable_storable<T&>` was false, or vice versa.
So I suppose no immediate action is needed here; let's just see if anyone files a bug.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.swap.pass.cpp:142
+  HasIterSwap a(value1), b(value2);
+  std::ranges::iter_swap(a, b);
+  assert(value1 == 1 && value2 == 1);
----------------
This is the only place you test `std::ranges::iter_swap(lvalue, lvalue)`. Please expand the tests below to test them also with lvalues of type `HasRangesSwapWrapper`, `A*`, etc., at least enough to hit each different overload of `iter_swap` once with at least one lvalue.

You never test `std::ranges::iter_swap(lvalue, rvalue)` or `std::ranges::iter_swap(rvalue, lvalue)`, but I don't think those need testing, as long as you hit the `(lvalue, lvalue)` cases.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/unqualified_lookup_wrapper.h:70-71
+
+  constexpr move_tracker(move_tracker const& other) = delete;
+  constexpr move_tracker& operator=(move_tracker const& other) = delete;
+
----------------
```
move_tracker(const move_tracker&) = delete;
move_tracker& operator=(const move_tracker&) = delete;
```
(Deleted functions don't need `constexpr`.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102809