[libcxx-commits] [PATCH] D102809: [libcxx][ranges] Add `ranges::iter_swap`.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed May 19 17:25:55 PDT 2021
Quuxplusone added inline comments.
================
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
----------------
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.
================
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);
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102809/new/
https://reviews.llvm.org/D102809
More information about the libcxx-commits
mailing list