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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I'm not done with the review, but I figured I might as well give you the feedback I have right away.

It generally looks good, but I think you might have forgotten about `n < 0` in both algorithms.



================
Comment at: libcxx/include/algorithm:310
+template<class ForwardIterator>
+  constexpr ForwardIterator
+    shift_right(ForwardIterator first, ForwardIterator last,
----------------
`shift_right` isn't `constexpr` in the paper (haven't looked at why yet). We're not allowed to add this as an extension.


================
Comment at: libcxx/include/algorithm:3269
+template <class _ForwardIterator>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator
----------------
Just `constexpr` should be OK, since this is only enabled after C++17 anyway.


================
Comment at: libcxx/include/algorithm:3274
+{
+    if (__n == 0) {
+        return __last;
----------------
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.


================
Comment at: libcxx/include/algorithm:3296
+template <class _ForwardIterator>
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
+_ForwardIterator
----------------
`inline` is unnecessary here, a template already has the right linkage. Applies above too.


================
Comment at: libcxx/include/algorithm:3310
+        }
+        _ForwardIterator __m = __first + (__d - __n);
+        return _VSTD::move_backward(__first, __m, __last);
----------------
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.


================
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)
----------------
Do you mean `an __n-element window [__trail, __lead)` instead (half-open interval)?


================
Comment at: libcxx/include/algorithm:3336-3361
+        auto __trail = __first;
+        auto __lead = __ret;
+        while (__trail != __ret) {
+            if (__lead == __last) {
+                _VSTD::move(__first, __trail, __ret);
+                return __ret;
+            }
----------------
Note to self: haven't reviewed this in detail yet.


================
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,
----------------
Remove `constexpr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93819