Fixed issue when recursively creating Market objects #904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pietfried
requested review from
hikinggrass and
corneliusclaussen
as code owners
a-w50
reviewed
Oct 11, 2024
| @@ -186,7 +186,7 @@ std::vector<types::energy::EnforcedLimits> EnergyManager::run_optimizer(types::e | |||
| optimized_values.reserve(brokers.size()); | |||
|
|
|||
| for (auto broker : brokers) { | |||
| auto local_market = broker->get_local_market(); | |||
| auto& local_market = broker->get_local_market(); | |||
modules/EnergyManager/Market.hpp
Outdated
| @@ -77,7 +76,7 @@ class Market { | |||
|
|
|||
| private: | |||
| Market* _parent; | |||
| std::vector<Market> _children; | |||
| std::vector<std::unique_ptr<Market>> _children; | |||
There was a problem hiding this comment.
I'm not sure why you added std::unique_ptr here - I would appreciate if you mention this in the commit message (what was the failure and how it got solved).
I guess, the Market objects don't behave well, when the vector gets enhanced or references are taken to these elements and become dangling when the vector gets resized. If this is the case, a simpler solution would be to use std::list instead of std::vector in combination with std::unique_ptr. std::list also guarantees, that it's elements stay at the same address, when other elements get added or removed.
There was a problem hiding this comment.
Thanks, changed it to std::list and added the reasoning in the PR description
…of children to unique_ptr Signed-off-by: Piet Gömpel <[email protected]>
* switched from std::vector to std::list type for Markets Signed-off-by: Piet Gömpel <[email protected]>
Pietfried
force-pushed
the
fix/energymanager-multiple-market-children
branch
from
a97c521 to
827e50f
Compare
Pietfried
changed the title
corneliusclaussen
approved these changes
Oct 15, 2024
corneliusclaussen
deleted the
fix/energymanager-multiple-market-children
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
The Market class previously used a
std::vector<Market> _childrento store childMarketobjects, which were initialized recursively within theMarketconstructor:The problem with that is that
std::vectormay need to resize and move its elements to a new memory location when additional elements are added. When this happens, all existingMarketelements are moved, which invalidates any pointers or references to those elements. In this case, the_parentpointer in could end up pointing to an invalid memory location, leading to segmentation faults when accessed.Changes
_childrenfromstd::vectortostd::list, as it guarantees that its elements remain at stable memory addresses even when new elements are inserted. This ensures that_parentpointers remain valid and point to the correct objectschildren()function because it was dead code and no longer needed.Issue ticket number and link
Checklist before requesting a review