[libcxx-commits] [PATCH] D60027: Add shift functions (P0769)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/algorithm:644
+template<class ExecutionPolicy, class ForwardIterator>
+    ForwardIterator shift_right(ExecutionPolicy&& exec, ForwardIterator first, ForwardIterator last,
+                                typename iterator_traits<ForwardIterator>::difference_type n);
----------------
You shouldn't implement the parallel version here, that should be in PSTL.


================
Comment at: libcxx/include/algorithm:5693
+template<class _It>
+_LIBCPP_CONSTEXPR_AFTER_CXX14 _It
+shift_left(_It __first, _It __last, typename iterator_traits<_It>::difference_type __n)
----------------
This should just be `_LIBCPP_CONSTEXPR` since it's a C++20 algorithm anyway.


================
Comment at: libcxx/include/algorithm:5724
+_LIBCPP_CONSTEXPR_AFTER_CXX14 _It
+__n_before_end(_It __first, _It __last, _St __n)
+{
----------------
Is this equivalent to `std::advance(__first, std::distance(__first, __last) - __n)`? If so, why not use that?


================
Comment at: libcxx/include/algorithm:5734
+_LIBCPP_CONSTEXPR_AFTER_CXX14 _It
+shift_right(_It __first, _It __last, typename iterator_traits<_It>::difference_type __n)
+{
----------------
We're making multiple passes over the whole range to get things like `__rbegin`. I can't imagine that's the only way of implementing this algorithm, and if so, that was certainly not the intent of the paper.

Did you look at https://github.com/danra/shift_proposal/blob/master/shift_proposal.h?


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

https://reviews.llvm.org/D60027