var-const Mordante ldionne
- Group Reviewers
- rGef8e91826126: [libc++] Forward more often to memmove in copy
Is it possible to add some tests for this? For checking the optimization, perhaps use structs containing pointers to dynamically-allocated memory that have deep-copying constructors, then check that only the pointers themselves were copied?
Nit: I think checking _InIter should happen before _OutIter for consistency with the argument order.
Question: why wouldn't the other reverse_iter overload that calls __unwrap_iter handle the case?
I don't think there is a way to check other than grepping for memmove in the binary. IIUC you suggestion would be to add non-trivial assignment operators, but that would disable the optimization. The optimization should only apply if it's invisible to the user.
With a reverse_iterator<reverse_iterator<int*>> the other overload would have _InIter = reverse_iterator<int*>. This is only a random access iterator and not a contiguous iterator, so it wouldn't get unwrapped.
Not sure if this helps, but Chrome noticed this breakage during an attempt to roll libc++: https://crbug.com/1322961
The "test" that we have for this is quite horrible and I'm not sure that we'd want to adopt it here, but just for the sake of completeness: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/checked_iterators_unittest.cc;l=101;drc=633d2d745d1fd09256ab846e7f923fbc5c084770
This is one of the reasons why I like sharing implementations between ranges versions of algorithms and non-ranges versions. Even a simple algorithm like std::copy can become not-so-simple due to subtle reasons, and we'd risk not fixing/improving one of the two algorithms if we had two copies. Nothing to do about this comment, just a rationale for why I can appear annoying with my requests to share ranges and non-ranges code.
Is it possible that what you're looking for here is instead is_contiguous_iterator<_Iter> && is_trivially_copy_assignable<value_type<_Iter>>? I don't think we have a trait for is_contiguous_iterator that works with older standards and understands __wrap_iter properly, but we probably should if that's indeed what you mean.
Can you negate the condition used below instead? It makes it easier to understand what's going on:
__enable_if_t<!(is_copy_constructible<_InIter>::value && is_copy_constructible<_Sent>::value && is_copy_constructible<_OutIter>::value)>
Can you use class = __enable_if_t<...> instead to allow placing the return type in the usual place? Note that if you run into duplicate declaration issues, you can use non-type template parameters instead:
template <class _InIter, class _Sent, class _OutIter, __enable_if_t<CONDITION>* = nullptr> ...
This needs a comment explaining what the test does (it tests that we optimize copy operations into memmoves IIUC).
LGTM, but let's fix the failing CI tests. Thanks!
I would like to investigate (in a different patch if the experiment turns out positive) the possibility of overloading __unwrap_iter (and __rewrap_iter) for reverse_iterator<reverse_iterator<It>> more generally. If that works out, we would get some added benefits in other algorithms as well for free, and this overload could go away entirely.