[libcxx-commits] [PATCH] D93819: [libc++] Implement [P0769] "Add shift to algorithm" (shift_left, shift_right)
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jan 18 16:01:34 PST 2021
Quuxplusone marked 7 inline comments as done.
Quuxplusone added inline comments.
================
Comment at: libcxx/include/algorithm:310
+template<class ForwardIterator>
+ constexpr ForwardIterator
+ shift_right(ForwardIterator first, ForwardIterator last,
----------------
ldionne wrote:
> `shift_right` isn't `constexpr` in the paper (haven't looked at why yet). We're not allowed to add this as an extension.
The P-numbered paper doesn't match what was actually landed in http://eel.is/c++draft/alg.shift .
What was landed looks //vastly// more "C++ library-esque": constexpr in the right places, more UB instead of special-case defined behavior, etc.
================
Comment at: libcxx/include/algorithm:3274
+{
+ if (__n == 0) {
+ return __last;
----------------
ldionne wrote:
> Should we use `n <= 0` here instead? This would make it clearer that nothing happens when `n < 0`.
>
> And actually, it seems to me that the random access iterator path doesn't behave correctly when `n < 0`. Indeed, we'll move `[__first + __n, __last)` (where `__first + __n` lies *before* `__first` since `__n` is negative) into `__first`. If I'm not mistaken, please fix and add tests for this to both algorithms.
http://eel.is/c++draft/alg.shift `n >= 0` is a precondition on both algorithms.
================
Comment at: libcxx/include/algorithm:3296
+template <class _ForwardIterator>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator
----------------
ldionne wrote:
> `inline` is unnecessary here, a template already has the right linkage. Applies above too.
This is how all of the algorithms are done, though, from `all_of` on line 852 all the way to `prev_permutation` on line 5840. I don't know why they all use `inline`, but `shift_right` shouldn't be a special case.
================
Comment at: libcxx/include/algorithm:3310
+ }
+ _ForwardIterator __m = __first + (__d - __n);
+ return _VSTD::move_backward(__first, __m, __last);
----------------
ldionne wrote:
> If `__n` is negative, `__m` is out-of-bounds, and the call to `move_backwards` is UB anyway because `last` is inside `(first, m]`. Please fix and add tests to cover this case.
http://eel.is/c++draft/alg.shift `n >= 0` is a precondition.
================
Comment at: libcxx/include/algorithm:3331
+ // We have an __n-element scratch space from __first to __ret.
+ // Slide an __n-element window (__trail, __lead) from left to right.
+ // We're essentially doing swap_ranges(__first, __ret, __trail, __lead)
----------------
ldionne wrote:
> Do you mean `an __n-element window [__trail, __lead)` instead (half-open interval)?
Yes. Also, it's not //really// "sliding"; it's kind of... leapfrogging? Glitching? We process the window `[0, n)` and then `[n, 2*n)` and then `[2*n, 3*n)` and so on. So this description is already kind of askew. But I'll happily change `(` to `[`, anyway.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.shift/shift_right.pass.cpp:14
+// template<class ForwardIterator>
+// constexpr ForwardIterator
+// shift_right(ForwardIterator first, ForwardIterator last,
----------------
ldionne wrote:
> Remove `constexpr`
Not done because I assume this comment was based on the P-numbered paper (the standard standard does have `constexpr`)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93819/new/
https://reviews.llvm.org/D93819
More information about the libcxx-commits
mailing list