-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memory management in Variants #502
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not yet checked if it fixes the actual bug, but in principle it looks good to me. When I have verified the fix and my comments are addressed, I am happy to merge.
@@ -19,8 +19,7 @@ void Module::setDescription(const std::string &d) { | |||
|
|||
AbstractCondition::AbstractCondition() : | |||
makeRejectedInactive(true), makeAcceptedInactive(false), rejectFlagKey( | |||
"Rejected") { | |||
|
|||
"Rejected"), rejectFlagValue( typeid(*this).name() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow to check which module deactivated the candidate, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the idea. This function was already implemented earlier but the value was not set correctly. I think the module name as default is a good starting option, without going through all modules and set it by hand.
The error also occurred for setting a property to a candidate, but only if the property is of type STRING.
will give the same issue as discussed before. With the explicit delete this issue is solved. The only remaining point is related to the Python gives the warning
One solution could be to change the |
Hi @JulienDoerner |
fixes #493:
The
abstractConditions
added a property to the candidate on the rejection. Up to now it only adds the keyRejected
. I added the module name as the default Value for the flag.Also the variant (in which this key is stored), does not have the right destructor. If it is deleted, the type of the variant was not passed to the
clear
function. Therefore no deletion of the variant was possible. This leaded to the bug in the memory consumption.