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

[Louis Dionne via Phabricator via libcxx-commits]

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

I am quite happy with this patch. There are a few nits, but other than that, LGTM. Thanks for the numerous iterations offline on this one.

@philnik I'll let you give final approval once you're satisfied.



================
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;
 }
----------------
philnik wrote:
> Why does `copy` get an `_AlgPolicy` now? AFAICT it's never used anywhere, which means it just produces duplicate code.
For consistency.

To address the code size concern, I think what we should do is change `__copy` above like so:

```
template <class _AlgPolicy, class _InIter, class _Sent, class _OutIter>
pair<_InIter, _OutIter>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
__copy(_InIter __first, _Sent __last, _OutIter __result) {
  // NOTE: There is no difference between the ranges and the STL classic version of this algorithm,
  //       so we always use _ClassicAlgPolicy to reduce code bloat.
  return std::__dispatch_copy_or_move<__copy_loop<_ClassicAlgPolicy>, __copy_trivial<_ClassicAlgPolicy> >(
      std::move(__first), std::move(__last), std::move(__result));
}
```


================
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));
+
----------------
philnik wrote:
> Why did you get rid of calling `__builtin_memmove` during constant evaluation?
GCC didn't support it, which added a bunch of complexity. So now what we do instead is unwrap the iterators and use the loop version when we're inside constant evaluation. The performance during constant evaluation is less of a concern than performance at runtime.


================
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; }
 };
----------------
var-const wrote:
> ldionne wrote:
> > Not sure whether the reordering is worth doing. Neutral on this.
> In that case, I'd keep it -- I think it makes more sense to place _re_wrapping after unwrapping.
Please land it as a separate NFC patch, that should solve the issue and close this thread.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
var-const wrote:
> I think these tests can be made to work pre-C++20, but it would be painful, partially because `contiguous_iterator_tag` is a C++20 addition. Please let me know if you think it's worth the trouble.
I don't think it's worth the trouble.


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:50
+
+static_assert(!std::is_trivially_assignable<NonTrivialMove&, NonTrivialMove&>::value);
+
----------------



================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:64
+
+static_assert(!std::is_trivially_assignable<NonTrivialCopy&, NonTrivialCopy&>::value);
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695