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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked an inline comment as done.
var-const added inline comments.


================
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);
----------------
Quuxplusone wrote:
> 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.
`__memory/ranges_uninitialized_algorithms.h` was introduced by my first patch in this series. It deliberately delegates most of the work to the internal functions contained in `__memory/uninitialized_algorithms.h`.

I have considered maintaining separate implementations before starting on this path. The amount of code duplication would be significant, with all the associated drawbacks. Already in `uninitialized_algorithms.h` before these changes there was a lot of inconsistency between very similar functions, this would only grow worse with twice the duplication and code being split among two different files.

> 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)

Done.


================
Comment at: libcxx/include/memory:185
+template<class InputIterator, class OutputIterator>
+using ranges::uninitialized_copy_result = in_out_result<InputIterator, OutputIterator>; // since C++20
+
----------------
Quuxplusone wrote:
> 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`).
Done (throughout the file).

Another question on this topic -- I've seen a few instances where lines in the synopsis run longer than even 120 characters. Is this intentional or just an oversight?


================
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" }
----------------
Quuxplusone wrote:
> (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//.
> If there are more types that need to be rolled into this same header, we can always rename it later if needed.

There are a plenty of very similar closely-related types:
```
in_fun_result
in_in_result
in_out_result
in_in_out_result
in_out_out_result
min_max_result
in_found_result
```

Given how small and similar they are, it seemed to me like consolidating them in a single file makes sense. Do you feel these should be separated?


================
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)>);
----------------
Quuxplusone wrote:
> 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.
I would prefer having a single test file devoted to this as well. Added a TODO throughout.


================
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));
----------------
Quuxplusone wrote:
> 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 )
Thank you for the link. While I can see the benefit described there, this form also makes me wonder whether `forward_iterator` is a type or a function, and if it's the latter, then what kind of a return type it might have. This makes me hesitant to make this change.


================
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);
----------------
Quuxplusone wrote:
> 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.
> except that `in` has 5 elements when it probably was intended to have only 3?

I wanted to have a test case where only a part of the output buffer is used (here and in similar recently-committed tests). If you feel it doesn't add value, I can reduce the size of the buffer to `3`.


================
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();
+
----------------
Quuxplusone wrote:
> 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.
Thanks for spotting this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116023