[libcxx-commits] [PATCH] D116023: [libc++][ranges] Implement `uninitialized_copy{, _n}` and `uninitialized_move{, _n}`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a subscriber: ldionne.
var-const added inline comments.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:28
+// language mode.
+struct __unreachable_sentinel {
+  template <class _Iter>
----------------
This class is to allow the non-ranges versions of algorithms operating on two sequences to use the same internal implementation (because the non-ranges versions only provide the beginning of the output range, so some placeholder is necessary for the end of the output range).


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:39
+template <class _ValueType, class _InputIterator, class _Sentinel1, class _ForwardIterator, class _Sentinel2>
+inline _LIBCPP_HIDE_FROM_ABI pair<_InputIterator, _ForwardIterator>
+__uninitialized_copy(_InputIterator __ifirst, _Sentinel1 __ilast, _ForwardIterator __ofirst, _Sentinel2 __olast) {
----------------
Note: `__compressed_pair` cannot be used here because it doesn't support its two types being the same type. `tuple`, which supports EBO, also cannot be used here because `uninitialized_copy` must be available in the C++03 mode.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:41
+__uninitialized_copy(_InputIterator __ifirst, _Sentinel1 __ilast, _ForwardIterator __ofirst, _Sentinel2 __olast) {
+  _ForwardIterator __idx = __ofirst;
 #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
I made some refactorings throughout to make the implementations in this file to be more similar to each other (using same names and similar code flow).


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:45
 #endif
-        for (; __f != __l; ++__f, (void) ++__r)
-            ::new ((void*)_VSTD::addressof(*__r)) value_type(*__f);
+    for (; __ifirst != __ilast && !(__olast == __idx); ++__ifirst, (void)++__idx)
+      ::new (_VSTD::__voidify(*__idx)) _ValueType(*__ifirst);
----------------
>From a quick look at the Standard's definition of `unreachable_sentinel`, it only defines `operator==` (not `!=`) and the sentinel is the first argument. I didn't want to expand the interface, hence the somewhat convoluted comparison here. However, maybe I'm missing something and other forms of comparison are synthesized somehow for `unreachable_sentinel`?


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:280
+template <class _ValueType, class _InputIterator, class _Sentinel1, class _ForwardIterator, class _Sentinel2,
+          class _Move>
+inline _LIBCPP_HIDE_FROM_ABI pair<_InputIterator, _ForwardIterator>
----------------
There is an important implementation difference between the ranges- and non-ranges versions of `uninitialized_move`: the old version simply calls `std::move` on each element, while the ranges version calls `std::ranges::iter_move`, which is a customization point. To avoid maintaining two parallel implementations, use a functor.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:281
+          class _Move>
+inline _LIBCPP_HIDE_FROM_ABI pair<_InputIterator, _ForwardIterator>
+__uninitialized_move(_InputIterator __ifirst, _Sentinel1 __ilast, _ForwardIterator __ofirst, _Sentinel2 __olast,
----------------
Note: I could use `tuple` here to get EBO (suggested by @ldionne). I'm a little concerned about consistency with the very similar `uninitialized_copy` algorithm (though it can be argues that `uninitialized_move` already diverges quite a bit). Happy to change to `tuple` if other people feel it's appropriate here.


================
Comment at: libcxx/include/memory:185
+template<class InputIterator, class OutputIterator>
+using ranges::uninitialized_copy_result = in_out_result<InputIterator, OutputIterator>; // since C++20
+
----------------
For entities in the nested `ranges` namespace, is it preferable to specify them like this:
```
template <class A, class B>
ranges::foo_result ranges::foo(A a, B b);
```
or like this:
```
namespace ranges {
template <class A, class B>
foo_result foo(A a, B b);
}
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116023