Use std::vector instead of std::map in BaseStorage #831
Conversation
Signed-off-by: Nate Koenig <natekoenig@gmail.com>
| { | ||
| return this->store.end(); | ||
| } | ||
| return ConstIterByIndex(idx->second); |
There was a problem hiding this comment.
I see we're returning iterators. If the iterators are being stored somewhere, we need to be careful since insertion to the std::vector might invalidate it. This was not the case for std::map, so we might be introducing subtle bugs here.
There was a problem hiding this comment.
Since the original implementation was based on a map, it makes sense to keep iters and pass them around. Now that we're changing to vector, I think we don't need to be doing this as much. The only place that use these IterByName functions internally is ContainsName, which can by re-implemented easily.
So how about we deprecate ConstIterByName / IterByName functions, update doxygen comment for these 2 APIs to warn users not to keep a handle to the iterator as it may change, and add a note to migration guide?
If that sounds good, I can help with this in a follow-up PR.
There was a problem hiding this comment.
ok I created a PR: #832
I didn't deprecate the functions. Instead I added doc with a note that iterators can change / become invalid. I also updated migration guide.
|
|
fixed in #832 |
* add doc and fix test Signed-off-by: Ian Chen <ichen@openrobotics.org>
Codecov Report
@@ Coverage Diff @@
## main #831 +/- ##
==========================================
+ Coverage 75.29% 75.31% +0.02%
==========================================
Files 177 177
Lines 16020 16027 +7
==========================================
+ Hits 12062 12071 +9
+ Misses 3958 3956 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@azeey , are you okay with this PR? |
|
If Ian has approved it, it's good with me. I was just quickly looking through and noticed iterators being returned and alarm bells went off in my head. |
🎉 New feature
Summary
Most of the time the
storestd::map inside BaseStorage is iterated over, which is not as efficient as a std::vector.Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.