[libcxx-commits] [PATCH] D122982: [libc++] Implement ranges::copy{, _n, _if, _backward}
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Apr 4 18:10:03 PDT 2022
var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/__algorithm/copy.h:39-40
-template <class _InputIterator, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY
-_OutputIterator
-__copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result)
-{
- return _VSTD::__copy_constexpr(__first, __last, __result);
+template <class _ValueT,
+ class = __enable_if_t<!is_const<_ValueT>::value && is_trivially_copy_assignable<_ValueT>::value> >
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
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?
================
Comment at: libcxx/include/__algorithm/copy.h:44
+ if (__libcpp_is_constant_evaluated()
+#ifndef _LIBCPP_COMPILER_GCC
+ && !is_trivially_copyable<_ValueT>::value
----------------
Can you please add a comment to explain the GCC workaround?
================
Comment at: libcxx/include/__algorithm/copy.h:51
+ if (__n > 0)
+ __builtin_memmove(__result, __first, __n * sizeof(_ValueT));
+ return pair<_ValueT*, _ValueT*>(__first + __n, __result + __n);
----------------
Question: what's the advantage of using `__builtin_memmove` directly?
================
Comment at: libcxx/include/__algorithm/copy.h:56
+template <class _InIter, class _Sent, class _OutIter>
+inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
+pair<_InIter, _OutIter> __copy(_InIter __first, _Sent __last, _OutIter __result) {
----------------
Question: what's the reason to change from `_LIBCPP_CONSTEXPR_AFTER_CXX17` to `_LIBCPP_CONSTEXPR_AFTER_CXX11`?
================
Comment at: libcxx/include/__algorithm/copy.h:58
+pair<_InIter, _OutIter> __copy(_InIter __first, _Sent __last, _OutIter __result) {
+ auto __ret = std::__copy_impl(std::move(__first), std::move(__last), std::move(__result));
+ return pair<_InIter, _OutIter>(std::move(__ret.first), std::move(__ret.second));
----------------
Question: can this simply `return std::__copy_impl ...`?
================
Comment at: libcxx/include/__algorithm/copy.h:69
+ auto __ret = std::__copy_impl(std::__unwrap_iter(__first), std::__unwrap_iter(__last), std::__unwrap_iter(__result));
+ return pair<_InIter, _OutIter>(std::__rewrap_iter(__first, __ret.first), std::__rewrap_iter(__result, __ret.second));
}
----------------
Question: any reason not to use `make_pair`?
================
Comment at: libcxx/include/__algorithm/copy.h:76
+copy(_InputIterator __first, _InputIterator __last, _OutputIterator __result) {
+ return std::__copy(__first, __last, __result).second;
}
----------------
Nit: move?
================
Comment at: libcxx/include/__algorithm/copy_backward.h:29
+pair<_Iter1, _Iter2> __copy_backward_impl(_Iter1 __first, _Sent1 __last, _Iter2 __result) {
+ auto __ret = std::__copy(reverse_iterator<_Sent1>(__last),
+ reverse_iterator<_Iter1>(__first),
----------------
Optional: `make_reverse_iterator` to omit the type?
================
Comment at: libcxx/include/__algorithm/copy_backward.h:39
+pair<_ValueT*, _ValueT*> __copy_backward_impl(_ValueT* __first, _ValueT* __last, _ValueT* __result) {
+ auto __distance = __last - __first;
+ std::__copy(__first, __last, __result - __distance);
----------------
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.
================
Comment at: libcxx/include/__algorithm/ranges_copy.h:32
+
+template <class _Ip, class _Op>
+using copy_result = in_out_result<_Ip, _Op>;
----------------
Nit: this should probably use longer names.
================
Comment at: libcxx/include/__algorithm/ranges_copy_backward.h:37
+
+ template <bidirectional_iterator _I1, sentinel_for<_I1> _S1, bidirectional_iterator _I2>
+ requires indirectly_copyable<_I1, _I2>
----------------
Mordante wrote:
> I would suggest to use `_Ip` and `_Op` or change the `copy_backward_result` above.
> In general this naming doesn't seem to use the new slightly longer names.
Nit: looks like these need to be renamed to something like `Iter1` or `InIter`.
================
Comment at: libcxx/include/__algorithm/ranges_copy_backward.h:41
+ copy_backward_result<_I1, _I2> operator()(_I1 __first, _S1 __last, _I2 __result) const {
+ auto __ret = std::__copy_backward(__first, __last, __result);
+ return {__ret.first, __ret.second};
----------------
Nit: move the arguments? (for consistency with `ranges_copy.h`)
================
Comment at: libcxx/include/__algorithm/ranges_copy_if.h:43
+ __go(_InIter __first, _Sent __last, _OutIter __result, _Pred& __pred, _Proj& __proj) {
+ for (; __first != __last; ++__first) {
+ if (std::invoke(__pred, std::invoke(__proj, *__first))) {
----------------
Question: why doesn't this algorithm delegate to the non-ranges version (is it to avoid having to deal with the projection)?
================
Comment at: libcxx/include/__algorithm/ranges_copy_n.h:26
+#endif
+
+
----------------
Nit: there's an extra blank line here (2 in a row).
================
Comment at: libcxx/include/algorithm:110
+ constexpr ranges::copy_if_result<borrowed_iterator_t<R>, O>
+ ranges::copy_if(R&& r, O result, Pred pred, Proj proj = {}); // since C++20
}
----------------
`ranges::copy_backward` seems to be missing.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:178
+ out[0].canCopy = true;
+ auto ret = std::ranges::copy(in, out.begin());
+ assert(ret.in == in.end());
----------------
Nit: in most of these tests, the ranges version goes after the iterator version.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:25
+
+static_assert(HasCopyIt<int*>);
+static_assert(!HasCopyIt<InputIteratorNotDerivedFrom>);
----------------
Nit: for consistency, `s/HasCopyIt/HasCopyBackwardIt` (throughout the file).
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:50
+template <class In, class Out, class Sent = In>
+constexpr void test_iterators() {
+ { // simple test
----------------
I think the current tests always use trivially copyable values -- can we add at least one test case where values are not trivially copyable?
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_backward.pass.cpp:154
+ struct OnlyBackwardsCopyable {
+ OnlyBackwardsCopyable* next = nullptr;
+ bool canCopy = false;
----------------
Optional: `s/next/prev/`?
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:114
+ {
+ std::array in = {4, 6, 87, 3};
+ std::array<int, 2> out;
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:115
+ std::array in = {4, 6, 87, 3};
+ std::array<int, 2> out;
+ auto ret = std::ranges::copy_if(In(in.data()),
----------------
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.
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp:162
+ std::array<S, 2> out;
+ auto ret = std::ranges::copy_if(in.begin(), in.end(), out.begin(), [](int i) { return i == 3; }, &S::val);
+ assert(ret.in == in.end());
----------------
Check the range version as well?
================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_n.pass.cpp:98
+
+ return true;
+}
----------------
Question: should this algorithm also check the order in which the elements are copied? (same with `copy_if.pass.cpp`)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122982/new/
https://reviews.llvm.org/D122982
More information about the libcxx-commits
mailing list