[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>
+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
More information about the libcxx-commits
mailing list