[libcxx-commits] [PATCH] D102809: [libcxx][ranges] Add `ranges::iter_swap`.
Tim Song via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed May 19 17:04:39 PDT 2021
tcanens added inline comments.
================
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);
----------------
Quuxplusone wrote:
> 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.
This is definitely `ranges::iter_move`.
================
Comment at: libcxx/include/__iterator/iter_swap.h:60
+ {
+ (void)iter_swap(_VSTD::forward<_T1>(__x), _VSTD::forward<_T2>(__y));
+ }
----------------
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.
================
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:
> > 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.
================
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))))
+ {
----------------
This is missing the assignment,
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