Feature/ranges part2 #8
Conversation
|
|
||
| /** | ||
| * @brief Make an IteratorView based on a container and a transformation | ||
| * To avoid copying containers like vectors, maps, etc. into the TransportView, |
There was a problem hiding this comment.
| * To avoid copying containers like vectors, maps, etc. into the TransportView, | |
| * To avoid copying containers like vectors, maps, etc. into the TransformView, |
There was a problem hiding this comment.
@whaeck don't forget to fix this typo
There was a problem hiding this comment.
I removed it, it's no longer there. But I'll check ...
src/tools/ranges/TransformView.hpp
Outdated
| * @param[in] range the range to be transformed | ||
| * @param[in] transform the transformation to be applied to the range | ||
| */ | ||
| constexpr TransformView( Range range, Transform transform ) : |
There was a problem hiding this comment.
Looked it up in cppref - I didn't know transform_view copied the data, I would have assumed it made a view of the data first. Is there any reason to not store an IteratorView of the range instead of copying the range itself? When I see a 'make_foo" function I expect it to basically forward along the args to the ctor of Foo. But in this case TransformView's ctor has different behavior than make_transform_view.
There was a problem hiding this comment.
I was as surprised as you were when I saw that here: https://github.com/CaseyCarter/cmcstl2. ranges v3 actually does create a subrange (i.e. IteratorView) for the transport_view. I was actually debating on putting an IteratorView into the TransportView to avoid copies.
There was a problem hiding this comment.
Are Iterator's methods tested somewhere?
There was a problem hiding this comment.
They are not. I forgot :-)
I'll add these.
|
|
||
| static_assert( | ||
| concepts::IsRandomAccessIterator< Iterator >::value == true, | ||
| "the at() method can only be made available for random access iterators" ); |
There was a problem hiding this comment.
Copy-paste errors on all of these static-asserts - they all reference the 'at()' method instead of the appropriate method.
Also, I may not have realized that doing this instead of SFINAE would require static asserts on all of the methods. Sorry about that.
There was a problem hiding this comment.
Ah, I see you fixed it in the next MR
There was a problem hiding this comment.
Indeed, I fixed it later.
I don;t mind all those static asserts. It;s more readable compared to sfinae
Adding a TransformView