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

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision as: huixie90.
huixie90 added a comment.

LGMT with nits and with green CI



================
Comment at: libcxx/include/__algorithm/copy.h:24
 
-// copy
+template <class _AlgPolicy>
+struct __copy_loop {
----------------
philnik wrote:
> Could you clang-format all the changes?
would it be possible to make this not a template?


================
Comment at: libcxx/include/__algorithm/copy.h:27-30
+  template <class _InIter, class _Sentinel, class _OutIter>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
+  pair<_InIter, _OutIter>
+  operator()(_InIter __first, _Sentinel __last, _OutIter __result) const {
----------------
philnik wrote:
> 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.
I understand it is not a problem but putting a completely unconstrained overload into the `__overload` struct makes me a bit concerned. I understand it is working right now because the other overload's signature takes a pointer, which is more specific than this one. Having said that, I don't think putting a constraint helps it in any way. 


================
Comment at: libcxx/include/__algorithm/copy.h:46
+__copy(_InIter __first, _Sent __last, _OutIter __result) {
+  return std::__dispatch_copy_or_move<__copy_loop<_AlgPolicy>, __copy_trivial<_AlgPolicy> >(
+      std::move(__first), std::move(__last), std::move(__result));
----------------
would it be possible to put `__copy_trivial` definition inside this header? AFAICT, the common header only reuses `__copy_trivial_impl` not `__copy_trivial` 


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:104-107
+struct __overload : _F1, _F2 {
+  using _F1::operator();
+  using _F2::operator();
+};
----------------
AFAIK, this is not an aggregate pre c++17 and you will need a constructor


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:109-118
+template <class _InIter, class _Sent, class _OutIter, class = void>
+struct __can_rewrap : false_type {};
+
+template <class _InIter, class _Sent, class _OutIter>
+struct __can_rewrap<_InIter, _Sent, _OutIter, __enable_if_t<
+    is_copy_constructible<_InIter>::value &&
+    is_copy_constructible<_Sent>::value &&
----------------
nit: would `_Lazy<_And<...` work here? 


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:142
+
+template <class _NaiveAlgorithm, class _OptimizedAlgorithm, class _InIter, class _Sent, class _OutIter>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17
----------------
extra nit and not an issue: I don't like the name `Naive` but I don't anything better


================
Comment at: libcxx/include/__algorithm/move.h:32
+    while (__first != __last) {
+      *__result = _IterOps<_AlgPolicy>::__iter_move(__first);
+      ++__first;
----------------
question: if the input is  a contiguous iterator pointing to a trivially copyable/moveable type, but the iterator has specialised `iter_move` ( yes it is very contrived), would the `move` dispatch to the overload that unwraps the  contiguous iterator to pointer and call `memmove` without going through the specialised `iter_move`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695