[libcxx-commits] [PATCH] D116023: [libc++][ranges] Implement `uninitialized_copy{, _n}` and `uninitialized_move{, _n}`.
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Dec 21 19:02:08 PST 2021
Quuxplusone added inline comments.
================
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,
----------------
ldionne wrote:
> 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.
EBO is only a concern when ABI is a concern. Here, if you're thinking about ABI, you've already lost. If this return value is ever materialized into memory, your perf is already shot to heck. (This is a general complaint of mine about C++20 Ranges //and// refactorings-to-use-more-helper-functions: the optimizer can paper over a lot of sins, but the flip side of that is that the library gradually becomes even less usable at `-O0`.)
================
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" }
----------------
var-const wrote:
> 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?
> 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?
I feel that there are two things to consider, and //right now// both of them militate in favor of `in_out_result.h`:
(1) .h files should be named for what's in them. What's in this file right now is `in_out_result`. Maybe that'll change in a few weeks; if it does, then we can change the file's name to match its new contents.
(2) Minimize dependencies: things should be combined if they're often used together, and split up if they're basically never used together. My ill-informed impression is that these `*_result` types are (2a) literally never used together, and (2b) in some cases, single-purpose types which really belong in their appropriate algorithm .h file; e.g. `min_max_result` probably belongs in `__algorithm/ranges_min_max.h` (and nowhere else).
================
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
----------------
ldionne wrote:
> This comment should clarify what "existing objects" means. Are we talking about the objects in the source range?
I assume it means "any of the existing objects in the //destination// range (which have not yet been overwritten)."
Line 224 says "the fourth object," but N is actually 3 and there's also a 5 involved, so I don't know which direction the off-by-one math is happening. IIUC, that comment could just say `Throw when constructing out[3]`.
Line 226 should be followed by an `assert(false)` (inside the try, but never hit because we expect to throw an exception).
And then circa line 229, we should do what the comment says we're doing: we should check that the value of `out[4].value` remains `5`. Without that check, I don't think this test is doing anything.
================
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));
----------------
var-const wrote:
> 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.
> While I can see the benefit described there, this form also makes me wonder whether `forward_iterator` is a type or a function
True. Of course this isn't unique to `forward_iterator<int*>`; we see that kind of ambiguity with `std::ranges::subrange` (type) on line 68 versus `std::ranges::uninitialized_copy` (function) on line 69.
Anyway, //I// think in this case it reads better with `=` instead of `()`; but I definitely can't claim that we use `=` 100% consistently.
Btw, if you're implying "Someone working on this file might not instantly understand that `forward_iterator<It>` is one of our test iterators from `test_iterators.h`," well, I //would// disagree with that implication. :) The test iterators are used in enough places that we should expect people to be at least as familiar with that vocabulary as with the `std::ranges` vocabulary (and hopefully more so!). I grant that `auto it = mary_poppins<int*>(in)` would be legitimately mysterious as to what kind of entity `mary_poppins` is; but `auto it = forward_iterator<int*>(in)` in this codebase shouldn't have quite that same level of mystery.
No action //needed// here, but if I've reduced your hesitancy, consider un-hesitating. ;)
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.copy/ranges_uninitialized_copy_n.pass.cpp:38-39
+struct NotConvertibleFromInt {};
+static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_copy_n), int*, size_t, NotConvertibleFromInt*,
+ NotConvertibleFromInt*>);
+
----------------
clang-format strikes again. I suggest
```
static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_copy_n),
int*, size_t, NotConvertibleFromInt*, NotConvertibleFromInt*>);
```
or even better, just
```
static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_copy_n), int**, size_t, int*, int*>);
```
since IIUC the point of this assertion is to test that it SFINAEs away when the source element type isn't convertible to the dest element type (e.g. `int` isn't convertible to `int*`).
================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.move/ranges_uninitialized_move.pass.cpp:71-75
+template <class T>
+T&& iter_move(Iterator<T>& iter) {
+ ++iter_move_invocations;
+ return std::move(*iter);
+}
----------------
I'd feel better if this were a non-template hidden friend:
```
friend T&& iter_move(Iterator it) {
++iter_move_invocations;
return std::move(*it);
}
```
Also, in writing that, I noticed that you were passing `it` by mutable reference on line 72. That's because https://eel.is/c++draft/specialized.algorithms#uninitialized.move-4 is overspecified, right? Personally I think we shouldn't bother to test that, and should file yet another LWG issue about the overspecification. This is the same overspecification problem as in `resize_and_overwrite`; and I hadn't noticed it before.
Intuition tells me that for the programmer to provide an ADL `iter_move` that works //only// with lvalues, might actually be IFNDR for some reason related to https://github.com/cplusplus/nbballot/issues/183 "implicit expression variations." But I certainly don't know for sure.
================
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());
----------------
ldionne wrote:
> 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!
+1... and note that in many existing tests, we don't even bother to pay the 2 spaces of indentation. ;)
================
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);
----------------
ldionne wrote:
> 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.
Pro tip: in tests that already require Concepts, this can be as simple as
```
std::same_as<std::ranges::in_out_result> auto result =
std::ranges::uninitialized_move_n(in, N, out.begin(), out.end());
```
although I guess that syntax works better for shorter types/expressions. :) Here it's not saving you much (if anything) over the old-school
```
auto result = std::ranges::uninitialized_move_n(in, N, out.begin(), out.end());
ASSERT_SAME_TYPE(decltype(result), std::ranges::in_out_result);
```
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