[libcxx-commits] [PATCH] D93819: [libc++] Implement [P0769] "Add shift to algorithm" (shift_left, shift_right)

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

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