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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: jloser, Quuxplusone.
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
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);
----------------
var-const wrote:
> 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`?
Yes, C++20's operator-spaceship feature includes the ability to create "rewritten candidates," so that `a < b` might actually call `(a <=> b) < 0` or `0 < (b <=> a)`, and `a != b` might actually call `!(a == b)` or `!(b == a)`.

My first impression here is that the benefits of sharing this code between C++11 and C++20 are outweighed by the costs, and we should just write a new file `<__memory/ranges_uninitialized_algorithms.h>` without trying to share code at all.

Actually, didn't we already do that? `<__memory/ranges_uninitialized_algorithms.h>` seems to exist! Can you double-check how this PR interacts with whatever PR introduced `<__memory/ranges_uninitialized_algorithms.h>` already?

However, you can at least simplify the C++11 codepath by making your new C++11-friendly `__unreachable_sentinel` provide `friend bool operator!=(const _Iter&, __unreachable_sentinel)` (and nothing else), because //that's// the unwritten syntactic requirement implied by the body of the (old) C++11 function template `__uninitialized_copy`. The template does require `(it != sent)` to compile, so your sentinel type should provide that operator. The template does //not// require `(sent == it)` to compile, so (to keep things simple) your sentinel type should //not// provide that operator.


================
Comment at: libcxx/include/memory:185
+template<class InputIterator, class OutputIterator>
+using ranges::uninitialized_copy_result = in_out_result<InputIterator, OutputIterator>; // since C++20
+
----------------
var-const wrote:
> 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);
> }
> ```
> ?
The precedent here is "do as https://eel.is/c++draft/memory.syn does" (which means reopen `namespace ranges`).


================
Comment at: libcxx/include/module.modulemap:223
       module adjacent_find            { private header "__algorithm/adjacent_find.h" }
+      module algorithm_results        { private header "__algorithm/algorithms_results.h" }
       module all_of                   { private header "__algorithm/all_of.h" }
----------------
(1, moot) Inconsistent pluralization here.
(2) Please call this `__algorithm/in_out_result.h` for now. If there are more types that need to be rolled into this same header, we can always rename it later //if needed//.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy.pass.cpp:33-36
+// Because this is a variable and not a function, it's guaranteed that ADL won't be used. However,
+// implementations are allowed to use a different mechanism to achieve this effect, so this check is
+// libc++-specific.
+LIBCPP_STATIC_ASSERT(std::is_class_v<decltype(std::ranges::uninitialized_copy)>);
----------------
FWIW, ultimately we'll probably want to move all these assertions into one centralized place similar to what @jloser did with the "customization point" CPOs and as I did with ADL-proofing and predicate-copying. Otherwise ultimately you're spending 4 lines in each of 100 different test files, and you probably end up missing a few dozen without noticing.
No action needed for now, though.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy.pass.cpp:60
+
+    forward_iterator<Counted*> it(in);
+    auto range = std::ranges::subrange(it, sentinel_wrapper<forward_iterator<Counted*>>(it));
----------------
Style nit: I'd prefer
```
auto it = forward_iterator<Counted*>(in);
```
for readability. ( https://quuxplusone.github.io/blog/2019/02/18/knightmare-of-initialization/#simple-guidelines-for-variable-i )


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy.pass.cpp:174-175
+
+    auto range = std::ranges::subrange(in, in + N);
+    auto view = std::ranges::reverse_view(range);
+    auto result = std::ranges::uninitialized_copy(view, out);
----------------
IIUC, these two lines could be just
```
auto view = in | std::views::reverse;
```
except that `in` has 5 elements when it probably was intended to have only 3? The last 2 of `in`'s elements aren't being used in this test.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy.pass.cpp:254-284
+  // Works with const iterators, (iter, sentinel) overload.
+  {
+    constexpr int N = 5;
+    Counted in[N] = {Counted(1), Counted(2), Counted(3), Counted(4), Counted(5)};
+    Buffer<Counted, N> out;
+    Counted::reset();
+
----------------
These two tests seem backwards to me. The important thing about "works with const iterators" is that it needs to work when the //destination// range has const iterators. Making the elements of the //source// range const — that also ought to work, sure, but it's significantly less interesting than making the elements of the //destination// range const.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116023