Details
- Reviewers
Mordante var-const ldionne jdoerfert - Group Reviewers
Restricted Project - Commits
- rG1d83750f631d: [libc++] Implement ranges::copy{, _n, _if, _backward}
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
In general looks good, several small issues.
libcxx/include/__algorithm/copy.h | ||
---|---|---|
84 | Do we need to have an additional specialization? where __enable_if_t<is_copy_constructible<_InIter>::value && is_copy_constructible<_Sent>::value && !is_copy_constructible<_OutIter>::value> > Just to make sure that a copy of a std::string to a non-copyable output iterator is still unwrapped. | |
libcxx/include/__algorithm/copy_backward.h | ||
56 | Did you have a look at the codegen for this change? | |
libcxx/include/__algorithm/ranges_copy.h | ||
34 | _InIter and _OutIter for consistency? | |
libcxx/include/__algorithm/ranges_copy_backward.h | ||
32 | :-) Happy you didn't follow the wording in the Standard. | |
38 | I would suggest to use _Ip and _Op or change the copy_backward_result above. | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
50 | The original is correct, but this is closer to the wording of the Standard. | |
libcxx/include/algorithm | ||
88 | Are the copy*result types already in the synopsis? | |
libcxx/test/libcxx/diagnostics/detail.headers/algorithm/ranges_copy.module.verify.cpp | ||
1 ↗ | (On Diff #420109) | These files will no longer be needed after D123028. |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp | ||
161 | Seems a copy paste. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
14 | Please add the synopsis. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp | ||
113 | Please count the number of invocations the predicated, same for the projection below. | |
151 | I would like to see tests for cpp20_output_iterator. |
libcxx/include/__algorithm/copy.h | ||
---|---|---|
39–79 | Hmm, if I'm reading this correctly, the previous condition supported copying from const T* to T*, whereas the new condition only supports T* to T*. Am I missing something? | |
44 | Can you please add a comment to explain the GCC workaround? | |
51 | Question: what's the advantage of using __builtin_memmove directly? | |
56 | Question: what's the reason to change from _LIBCPP_CONSTEXPR_AFTER_CXX17 to _LIBCPP_CONSTEXPR_AFTER_CXX11? | |
58 | Question: can this simply return std::__copy_impl ...? | |
95 | Nit: move? | |
libcxx/include/__algorithm/ranges_copy.h | ||
33 | Nit: this should probably use longer names. | |
libcxx/include/__algorithm/ranges_copy_backward.h | ||
38 | Nit: looks like these need to be renamed to something like Iter1 or InIter. | |
42 | Nit: move the arguments? (for consistency with ranges_copy.h) | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
44 | Question: why doesn't this algorithm delegate to the non-ranges version (is it to avoid having to deal with the projection)? | |
libcxx/include/__algorithm/ranges_copy_n.h | ||
27 | Nit: there's an extra blank line here (2 in a row). | |
libcxx/include/algorithm | ||
109 | ranges::copy_backward seems to be missing. |
libcxx/include/__algorithm/copy.h | ||
---|---|---|
88 | Question: any reason not to use make_pair? | |
libcxx/include/__algorithm/copy_backward.h | ||
29 | Optional: make_reverse_iterator to omit the type? | |
38 | Optional: I think it would be more readable to create another variable for __result_first or similar instead of calculating it on the fly below. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp | ||
179 | Nit: in most of these tests, the ranges version goes after the iterator version. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
26 | Nit: for consistency, s/HasCopyIt/HasCopyBackwardIt (throughout the file). | |
51 | I think the current tests always use trivially copyable values -- can we add at least one test case where values are not trivially copyable? | |
155 | Optional: s/next/prev/? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp | ||
115 | Nit: can you please add a few more even elements to the array? Currently, only the contiguous range in the beginning matches, which is a bit of a special case. Ideally, the matching elements should be more dispersed. | |
116 | I'd suggest making this array as large as in and relying on the return value to make sure only two elements are copied. Otherwise, it's not clear if the implementation would stop at two elements if the output array was larger. | |
163 | Check the range version as well? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp | ||
99 | Question: should this algorithm also check the order in which the elements are copied? (same with copy_if.pass.cpp) |
- Address comments
libcxx/include/__algorithm/copy.h | ||
---|---|---|
39–79 | No, I missed something. I thought the old version said is_same<remove_const<_Tp>::type, _Tp>, which is obviously just is_const. | |
51 | I can't use std::memmove in a constant expression and using __builtin_memmove costs a lot less steps (https://godbolt.org/z/nqb75KGa3). | |
56 | AFAIK the general style is to use the earliest constexpr possible for internal stuff. | |
84 | What would you expect to change between it being wrapped and not? It's not like we can optimize that in any way. | |
88 | I didn't know std::make_pair existed. Thanks! | |
95 | This patch exists in part because we want to avoid this exact extension, so no - this should definitely not be moved. See D122074. | |
libcxx/include/__algorithm/copy_backward.h | ||
56 | What change are you asking for exactly? The change from std::memmove to __builtin_memmove? | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
44 | Why would it? It's a really simple algorithm and has no pitfalls AFAIK. | |
50 | The problem is that your version isn't correct. _Sent doesn't have to be the same type as _InIter. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
51 | CopyOnce and OnlyBackwardsCopyable aren't trivial. | |
155 | I thought of it as 'the next copyable value'. I'd rather keep it as-is. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp | ||
116 | I'd argue that it should be exact. std::ranges::copy_if can't know the size of the range it gets passed, and if it would try to write to more elements than are available this will error during constant evaluation. i.e. an implementation could *(out + 1) = *in and you suggestion wouldn't catch that, while making the array wit the exact size would. | |
151 | I'll do that as soon as you land D122072. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp | ||
99 | for copy_if and copy_n it isn't specified in which order the elements are copied. |
- Remove std::make_reverse_iterator again
libcxx/include/__algorithm/copy_backward.h | ||
---|---|---|
29 | This isn't available in C++03 and C++11. |
libcxx/include/__algorithm/copy.h | ||
---|---|---|
44 | Sorry about nitpicking, but reading the comment, I think it would be better phrased as a TODO (something like Remove this workaround once GCC supports using __builtin_memmove in constexpr contexts). | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
44 | I don't disagree -- I'm just wondering because it's inconsistent with what ranges::copy does, which is to delegate. | |
libcxx/include/algorithm | ||
59–60 | Hmm, looks accidentally deleted? | |
196–197 | Looks accidentally deleted? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
51 | Yes, but those tests are focused on checking something else. I'd prefer to have a dedicated test for this, even if it's somewhat redundant with the other tests. |
libcxx/include/__algorithm/ranges_copy_if.h | ||
---|---|---|
44 | Yes, because std::copy does a few optimizations and isn't just a simple loop. | |
libcxx/include/algorithm | ||
59–60 | Looks like it, but has nothing to do with this PR. I'll fix it. | |
196–197 | I'll upload a fix. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
51 | What exactly do you want to check just with a non-trivially copyable type? If we do nothing inside the copy constructor/assignment operator there is nothing to check. The implementation could just as well memcpy the whole thing. |
libcxx/include/__algorithm/copy.h | ||
---|---|---|
46 | Do you know whether this is available in GCC-12? | |
84 | The cpp20 output iterator isn't copy constructible. But std::basic_string<CharT>::iterator can still be unwrapped. So I wondered whether we want an overload for that case. Something like template <class _InIter, class _Sent, class _OutIter, __enable_if_t<is_copy_constructible<_InIter>::value && is_copy_constructible<_Sent>::value> > inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 pair<_InIter, _OutIter> __copy(_InIter __first, _Sent __last, _OutIter __result) { auto __ret = std::__copy_impl(std::__unwrap_iter(__first), std::__unwrap_iter(__last), std::move__result)); return std::make_pair(std::__rewrap_iter(__first, __ret.first), _ret.second); } | |
libcxx/include/__algorithm/copy_backward.h | ||
56 | No the removal of this overload. Does that affect the generated code or not. | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
50 | I trusted the wording of the Standard, but it seems that's wrong ;-) {last, result + N} for the overloads in namespace ranges. Do you want to file an LWG issue? |
LGTM once the comment about a missing test case is addressed. Thanks a lot for working on this!
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
---|---|---|
51 | Right, this is one thing to check -- that the implementation doesn't try to memcpy non-trivial types, for example. But really, all I want to check is that the result is correct, not for any particular kind of problem because too many are possible to consider. Just the fact that it would produce the correct result without crashing is a valuable thing to test, even if it appears trivial. Furthermore, tests are written for the future as well. Even if an algorithm is "obviously" trivial and seemingly cannot be incorrect now, it could change in the future. |
libcxx/include/__algorithm/copy.h | ||
---|---|---|
46 | It's not available in GCC trunk. | |
84 | I think the unwrapping is mostly so that we can do a memmove if we have only raw pointers. I don't think that the unwrapping of only part of the iterators makes any difference. The compiler should still be able to see through the wrapper and optimize it away. | |
libcxx/include/__algorithm/copy_backward.h | ||
56 | The codegen is a bit worse, but I doubt it would even be measurable. It's two instructions more. https://godbolt.org/z/Pb7YbnsK3 | |
libcxx/include/__algorithm/ranges_copy_if.h | ||
50 | I'm not sure if this is an oversight or if this is expected. Most ranges algorithms return last according to the standard, while in practice they have to return first + N. It's the same with N itself. The standard says last - first, while in practice it would have to be std::distance(first, last). | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp | ||
51 | I don't disagree that it's a valuable test. My problem is that the test you propose is a strict subset of what // check that the range is copied backwards tests IIUC. I don't think the test adds any coverage. |
Something went wrong it seems you accidentally replaced this patch with the contents of D120637.
LGTM!
libcxx/include/__algorithm/copy_backward.h | ||
---|---|---|
29 | What I've done in the past for similar cases is add __make_reverse_iterator and let make_reverse_iterator call that function. Just something to keep in mind, no need to change it now. | |
libcxx/include/__algorithm/ranges_copy_n.h | ||
56 | How do you feel about moving these iterators for consistency? |
Can you please add a comment to explain the GCC workaround?