Optimize C++/WinRT's GetMany implementation#497
Conversation
kennykerr
requested review from
BenJKuhn,
DefaultRyan and
Scottj1s
as code owners
kennykerr
changed the title
| m_owner->copy_n(m_current, actual, values.begin()); | ||
| std::advance(m_current, actual); | ||
| return actual; | ||
| if constexpr (std::is_same_v<decltype(typename std::iterator_traits<iterator_type>::iterator_category()), std::random_access_iterator_tag>) |
There was a problem hiding this comment.
std::is_same is the wrong way to light up behavior for random access iterators, as it will improperly reject C++20's contiguous_iterator_tag. The iterator tags have an inheritance chain, basically representing the strength of the iterator guarantees - continuous_iterator_tag inherits from random_access_iterator_tag, which inherits from bidirectional_iterator_tag, and so on.
What you really need to do here is use overload dispatch to specialize the behavior based on iterator "strength", as in the example at https://en.cppreference.com/w/cpp/iterator/iterator_tags.
There was a problem hiding this comment.
Ah good idea. I'll do that.
| ++m_current; | ||
| } | ||
|
|
||
| return static_cast<uint32_t>(output - values.begin()); |
There was a problem hiding this comment.
Directly subtracting iterators is an operation that is only valid for random_access_iterator_tag, which this 'else' clause is explicitly not supposed to use. Do you have test coverage exercising this branch?
There was a problem hiding this comment.
Yes, this is heavily tested. In fact, I just added even more rigorous tests for this branch. #499
There was a problem hiding this comment.
I think the confusion here is that the subtraction is not on the input but on the output. 😉
There was a problem hiding this comment.
Oh yeah, that's totally safe to do on the output. Thanks for pointing that out. 👍
3 participants
The
GetManyimplementation forIIterable<T>was performing multiple traversals of the range when random access iterators weren't available. This update ensures that only a single traversal occurs in all cases and memcpy is used in more cases when the range offers random access.I've also moved the scratch test into a separate project as the tests have become too extensive.