[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`

[Nikolas Klauser via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__algorithm/copy.h:24
 
-// copy
+template <class _AlgPolicy>
+struct __copy_loop {
----------------
Could you clang-format all the changes?


================
Comment at: libcxx/include/__algorithm/copy.h:27
 
-template <class _InIter, class _Sent, class _OutIter>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
-pair<_InIter, _OutIter> __copy_impl(_InIter __first, _Sent __last, _OutIter __result) {
-  while (__first != __last) {
-    *__result = *__first;
-    ++__first;
-    ++__result;
+  template <class _InIter, class _Sentinel, class _OutIter>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
----------------
Why did you rename `_Sent` to `_Sentinel`? AFAIK we settled on `_Sent` for the naming in the ranges algorithms. Same for `_InputIterator` and similar stuff.


================
Comment at: libcxx/include/__algorithm/copy.h:54
 copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result) {
-  return std::__copy(__first, __last, __result).second;
+  return std::__copy<_ClassicAlgPolicy>(__first, __last, __result).second;
 }
----------------
Why does `copy` get an `_AlgPolicy` now? AFAICT it's never used anywhere, which means it just produces duplicate code.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:38
+
+    return pair<_InIter, _OutIter>(std::move(__original_last_iter), std::move(__result));
+  }
----------------
`std::make_pair`?


================
Comment at: libcxx/include/__algorithm/copy_backward.h:52
 template <class _BidirectionalIterator1, class _BidirectionalIterator2>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _BidirectionalIterator2
-copy_backward(_BidirectionalIterator1 __first, _BidirectionalIterator1 __last, _BidirectionalIterator2 __result) {
-  return std::__copy_backward<_ClassicAlgPolicy>(__first, __last, __result).second;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX20
+_BidirectionalIterator2
----------------
?!


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:20
+#include <cstring>
+#include <type_traits>
+
----------------
Could you use the granularized includes instead?


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:34
+  const size_t __n = static_cast<size_t>(__last - __first);
+  std::memmove(__result, __first, __n * sizeof(_Out));
+
----------------
Why did you get rid of calling `__builtin_memmove` during constant evaluation?


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:115
+    is_copy_constructible<_InIter>::value &&
+    is_copy_constructible<_Sent>::value &&
+    is_copy_constructible<_OutIter>::value
----------------
Sentinels are always copy-constructible.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:150
+
+  using _Algorithm = std::__overload<_NaiveAlgorithm, _OptimizedAlgorithm>;
+  return std::__unwrap_and_dispatch<_Algorithm>(std::move(__first), std::move(__last), std::move(__out_first));
----------------



================
Comment at: libcxx/include/__algorithm/move_backward.h:44
+template <class _AlgPolicy, class _BidirectionalIterator1, class _Sentinel, class _BidirectionalIterator2>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+pair<_BidirectionalIterator1, _BidirectionalIterator2>
----------------
Why constexpr since 20? Others are marked as constexpr since 14.


================
Comment at: libcxx/include/__algorithm/move_backward.h:56
               _BidirectionalIterator2 __result)
 {
+  return std::__move_backward<_ClassicAlgPolicy>(
----------------
Please add a `static_assert` to make sure the iterators are copyable.


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:36
   static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __unwrap(_Iter __i) _NOEXCEPT { return __i; }
+  static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __rewrap(_Iter, _Iter __iter) { return __iter; }
 };
----------------
Why did you move this code?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:34
 template <class _Iter, class _Sent>
+requires is_copy_constructible<_Iter>::value && is_copy_constructible<_Sent>::value
 struct __unwrap_range_impl {
----------------
AFAICT this isn't correct. Lines 43-45 should handle this case just fine. Also, why not `_v`?


================
Comment at: libcxx/include/__algorithm/unwrap_range.h:35
   _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Sent __sent)
-    requires random_access_iterator<_Iter> && sized_sentinel_for<_Sent, _Iter>
   {
----------------
Why the indentation change? This file was properly clang-formatted before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695