[libcxx-commits] [PATCH] D116023: [libc++][ranges] Implement `uninitialized_copy{, _n}` and `uninitialized_move{, _n}`.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Dec 21 11:35:20 PST 2021
ldionne accepted this revision.
ldionne added a comment.
Left some comments -- I don't need to see this again if you agree with my suggestions. I don't want to block you since I'll be out for the holidays soon.
================
Comment at: libcxx/include/__algorithm/algorithms_results.h:34
+ _OutputIterator2>
+ constexpr operator in_out_result<_InputIterator2, _OutputIterator2>() const & {
+ return {in, out};
----------------
`_LIBCPP_HIDE_FROM_ABI`
================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:240-241
+ requires constructible_from<range_value_t<_OutputRange>, range_reference_t<_InputRange>>
+ uninitialized_copy_result<borrowed_iterator_t<_InputRange>, borrowed_iterator_t<_OutputRange>> operator()(
+ _InputRange&& __in_range, _OutputRange&& __out_range) const {
+ // clang-format on
----------------
Suggestion, we often have the return type above the rest of the function. I'm not a big fan of that convention, but here it might help.
================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:44
#endif
- for (; __f != __l; ++__f, (void) ++__r)
- ::new ((void*)_VSTD::addressof(*__r)) value_type(*__f);
+ for (; __ifirst != __ilast && __idx != __olast; ++__ifirst, (void)++__idx)
+ ::new (_VSTD::__voidify(*__idx)) _ValueType(*__ifirst);
----------------
Note from our discussion: you noticed you are missing a test case in the ranges version when `__idx` reaches `__olast` before `__ifirst` reaches `__ilast`.
================
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,
----------------
var-const wrote:
> 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.
I am not utterly concerned by using `pair` and not doing EBO. I believe these algorithms will be used with stateful iterators where EBO wouldn't help most of the time anyway.
================
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:
> 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?
I don't think anyone has ever asked the question. I think we should just copy-paste the formatting of the Standard, as you seem to be doing already.
================
Comment at: libcxx/test/std/algorithms/algorithms.results/algorithms.results.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts, libcpp-has-no-incomplete-ranges
----------------
I suspect this test is going to have to be XFAILed for Clang on Windows, because I don't think the Clang emulation of MSVC supports `[[no_unique_address]]`. You can see whether that's correct once CI runs.
================
Comment at: libcxx/test/std/algorithms/algorithms.results/algorithms.results.compile.pass.cpp:23
+// Successful conversion.
+static_assert(std::is_convertible_v<std::ranges::in_out_result<int, long>, std::ranges::in_out_result<bool, double>>);
+
----------------
You seem to be missing a test case for when the members of `in_out_result` are move constructible vs copy constructible, i.e. the `const&` vs `&&` variant.
Also, there should be runtime tests actually running those conversion operators to make sure they are implemented correctly, even though I know it's an almost trivial class. (This means you'll need to move from `.compile.pass.cpp` to `.pass.cpp`)
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy.pass.cpp:215-216
+
+ // An exception is thrown while objects are being created -- the existing objects should stay
+ // valid. (iterator, sentinel) overload.
+#ifndef TEST_HAS_NO_EXCEPTIONS
----------------
This comment should clarify what "existing objects" means. Are we talking about the objects in the source range?
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.move/ranges_uninitialized_move.pass.cpp:86-91
+ auto result1 = std::ranges::uninitialized_move(in, in, out.begin(), out.end());
+ assert(Counted::current_objects == 0);
+ assert(Counted::total_objects == 0);
+ assert(Counted::total_copies == 0);
+ assert(result1.in == in);
+ assert(result1.out == out.begin());
----------------
Suggestion. It helps clarify that the test cases are 100% unrelated, and also allows skipping the numbering. All that for just two spaces of indentation!
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.move/ranges_uninitialized_move.pass.cpp:249-250
+
+ // An exception is thrown while objects are being created -- the existing objects should stay
+ // valid. (iterator, sentinel) overload.
+#ifndef TEST_HAS_NO_EXCEPTIONS
----------------
What http://eel.is/c++draft/specialized.algorithms#uninitialized.move-5 says is basically that if one of the move constructor throws, the objects in the source range (that have been moved-from) will now be left in a moved-from state (aka the valid-but-unspecified-state wording used in the spec).
Instead, IMO we should be asserting that the moved-from objects (i.e. `in[0]`, `in[1]`, `in[2]`) are indeed moved-from after the exception has been thrown.
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.move/ranges_uninitialized_move.pass.cpp:348
+
+ // `iter_move` is a customization point.
+ {
----------------
Here and elsewhere.
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.move/ranges_uninitialized_move_n.pass.cpp:99
+
+ auto result = std::ranges::uninitialized_move_n(in, N, out.begin(), out.end());
+ assert(Counted::current_objects == N);
----------------
I don't think I've seen tests that the various `std::ranges::` algorithms are returning exactly the type specified in the standard. If that's right, we should add some.
In other words, it's not sufficient to be returning some type that happens to have `.in` and `.out`, they need to be exactly what the spec says.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116023/new/
https://reviews.llvm.org/D116023
More information about the libcxx-commits
mailing list