[COM] Make IteratorType use std::vector instead of std::list #1931
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR changes the underlying container type in IteratorType from std::list to std::vector and updates the Add method to handle iterator invalidation that occurs when vector capacity changes.
- Container type changed from
std::listtostd::vector - Added logic to track and restore iterator position after potential vector reallocation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void Add(const typename INTERFACE::Element& element) | ||
| { | ||
| const bool wasValid = IsValid(); | ||
| const bool wasBeforeFirst = (_index == 0); |
There was a problem hiding this comment.
The condition _index == 0 doesn't correctly identify a before-first position. In most iterator implementations, index 0 represents the first element, not a before-first state. This should likely be _index < 1 or use a dedicated flag to track before-first state.
| const bool wasBeforeFirst = (_index == 0); | |
| const bool wasBeforeFirst = (_index < 1); |
There was a problem hiding this comment.
As far as I can tell, in our implementation _index is equal to zero at the creation of the iterator, thus the iterator is valid when _index is above zero
| else if (wasBeforeFirst == true) { | ||
| _iterator = _container.begin(); | ||
| } |
There was a problem hiding this comment.
When the iterator was in a before-first state and vector reallocation occurs, setting _iterator = _container.begin() moves it to point to the first element instead of maintaining the before-first state. This changes the iterator's logical position.
| else if (wasBeforeFirst == true) { | |
| _iterator = _container.begin(); | |
| } | |
| // else if (wasBeforeFirst == true) { | |
| // _iterator = _container.begin(); | |
| // } |
There was a problem hiding this comment.
Once again, in each ctor when the iterator is still empty, we set _iterator = _container.begin();, as well as _index(0)
| } | ||
|
|
||
| if (wasPastEnd == true) { | ||
| _iterator = _container.end(); | ||
| _index = Count() + 1; | ||
| } |
There was a problem hiding this comment.
This logic runs regardless of whether vector reallocation occurred. If no reallocation happened, the iterator is still valid and shouldn't be reset. This should be inside the capacity check block or have an additional condition to only execute when reallocation occurred.
| } | |
| if (wasPastEnd == true) { | |
| _iterator = _container.end(); | |
| _index = Count() + 1; | |
| } | |
| if (wasPastEnd == true) { | |
| _iterator = _container.end(); | |
| _index = Count() + 1; | |
| } | |
| } |
There was a problem hiding this comment.
If we were in a wasPastEnd state, then I think we should still be in that state, even after Adding a new element
Even if there was no reallocation, the _iterator would now point to the newly inserted element, and the new end() iterator is one past that element
But more importantly, if _index would still be equal to oldCount + 1, then it would now be equal to the new Count(), which would make it valid (IsValid() would return true). So adding a new element in an invalid state would validate that state
No description provided.