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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jan 18 14:29:09 PST 2021

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>
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` 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`

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list