Conversation
Improve safety by 100% Cached input: 608 μs Uncached input: 1,025 μs It is possible to implement a fix that preserves the cache characteristics to some degree, but it is quite more complex, and should be left for a future revision. Solves bemanproject#85
…e end Random-access iterators advance using a local offset while the erased underlying iterator remains unchanged. Sentinel comparison previously ignored that offset, so finite uncached ranges never reached their end.
patrickroberts
left a comment
patrickroberts
left a comment
There was a problem hiding this comment.
Overall, the additional test cases here are very valuable and I'd like to preserve those. I've left some comments on modifications to the implementation that need to be considered more carefully.
| if (this == std::addressof(other)) { | ||
| return *this; | ||
| if (this != std::addressof(other)) { | ||
| std::destroy_at(this); | ||
| std::construct_at(this, std::move(other)); | ||
| } | ||
|
|
||
| std::destroy_at(this); | ||
| std::construct_at(this, std::move(other)); |
There was a problem hiding this comment.
This change adds nothing
| std::destroy_at(this); | ||
| std::construct_at(this, other); | ||
| return *this; | ||
| return *this = basic_polymorphic(other); |
There was a problem hiding this comment.
For this and converting move assignment, would prefer to use explicit template parameters on basic_polymorphic to avoid relying on injected-class-name which could reasonably be interpreted as intending to perform CTAD, even though that's not the case here.
| using difference_type = std::ptrdiff_t; | ||
| using iterator_concept = std::forward_iterator_tag; | ||
|
|
||
| mutable int value{}; |
There was a problem hiding this comment.
Can you clarify why mutable is here? I'm not sure I see where this is needed.
| return *this; | ||
| } | ||
|
|
||
| int& operator*() const noexcept { |
There was a problem hiding this comment.
I'm sure you could make your point with const int& without mutable here.
| struct throwing_forward_view : std::ranges::view_base { | ||
| int* first{}; | ||
| int* last{}; | ||
|
|
||
| throwing_forward_view() = default; | ||
|
|
||
| throwing_forward_view(int* first, int* last) : first(first), last(last) {} | ||
|
|
||
| throwing_forward_iterator begin() const noexcept { return throwing_forward_iterator{first}; } | ||
|
|
||
| throwing_forward_iterator end() const noexcept { return throwing_forward_iterator{last}; } | ||
| }; |
There was a problem hiding this comment.
This can just be subrange<throwing_forward_iterator>
| struct self_ref_input_view : std::ranges::view_base { | ||
| int first{}; | ||
| int stop{}; | ||
|
|
||
| self_ref_input_view() = default; | ||
|
|
||
| self_ref_input_view(int first, int stop) : first(first), stop(stop) {} | ||
|
|
||
| self_ref_input_iterator begin() { return {first, stop}; } | ||
|
|
||
| std::default_sentinel_t end() const noexcept { return {}; } | ||
| }; |
There was a problem hiding this comment.
This can be subrange<self_ref_input_iterator, std::default_sentinel_t>
| self_ref_forward_proxy_iterator begin() const noexcept { return {first, stop}; } | ||
|
|
||
| self_ref_forward_proxy_iterator end() const noexcept { return {stop, stop}; } | ||
| }; |
There was a problem hiding this comment.
This can be subrange<self_ref_forward_proxy_iterator>
| template <class RefT, class RValueRefT> | ||
| struct input_cache_protocol : inherit<dereference_t<RefT>, iter_move_t<RValueRefT>, increment_t> {}; | ||
|
|
||
| template <has_cache RefT, class RValueRefT> | ||
| requires convertible_to_borrowed<rvalue_ref_t<RefT>, RValueRefT> | ||
| struct input_cache_protocol<RefT, RValueRefT> : inherit<cache_t<RefT>, next_t<RefT>> {}; |
There was a problem hiding this comment.
This and related changes to restrict where caching is applied need to be reverted. *i is required to be equality-preserving on an iterator, so it is semantically valid to cache it. It does not need to be a forward iterator.
| requires forward | ||
| = default; | ||
|
|
||
| constexpr iterator(iterator&&) noexcept = default; |
There was a problem hiding this comment.
What actually needs to change is here: the move construction of the polymorphic iterator invalidates the cache, so cache_t needs to be dispatched here to revalidate it, as well as in move assignment if that doesn't already delegate to move construction.
2 participants
Fixes #85
Fixes #86
Fix the bug with the random_access containers where we never reach the end