Use std::shared_ptr in controller settings to fix memory leak #14413
Conversation
| const QList<LegacyControllerMapping::ScriptFileInfo>& scripts) { | ||
| QList<LegacyControllerMapping::ScriptFileInfo> scripts) { |
There was a problem hiding this comment.
I think the actually generated a clazy warning because QList is too large?
There was a problem hiding this comment.
I don't think so, because it is kept inside the object. So a copy-by value is here the preferred signature.
| static std::shared_ptr<LegacyControllerBooleanSetting> createFrom(const QDomElement& element) { | ||
| return std::make_shared<LegacyControllerBooleanSetting>(element); |
There was a problem hiding this comment.
slight nitpick: creating a shared_ptr from a unique_ptr is easy, the other way around is hard. So unless we are sure that all consumers will always need shared ownership, prefer that first... I'll leave it up to you to make that evaluation.
There was a problem hiding this comment.
In Mixxx we always need a shared_ptr. We don't have shared pointers in tests.
I like to benefit here form std::make_shared which allocates the object and the control structure for the shared pointer in one go.
| bool (*matcher)(const QDomElement&); | ||
| AbstractLegacyControllerSetting* (*builder)(const QDomElement&); | ||
| std::function<bool(const QDomElement&)> matcher; | ||
| std::function<std::shared_ptr<AbstractLegacyControllerSetting>(const QDomElement&)> builder; |
There was a problem hiding this comment.
Can you explain your rationale here? This replaces an non-owning pointer type with an owning one and I don't see why that would need to be the case. The only real usecase where std::function is needed is when you need to pass a lambda with owning captures afaik.
There was a problem hiding this comment.
I have used std::function here, because it allows to take functions that are returning sub classes of AbstractLegacyControllerSetting.
Before I had only luck using an ugly reinterpret_cast or an extra wrapper function.
Is there a downside of using std::function compared to a wrapper for type conversion?
There are remaining objects not deleted in
controller_mapping_settings_test.cppI have fixed it by using std::shared_ptr from the very beginning by std::make_shared. In Mixxx itself the objects are later managed by a std::shared_ptr anyway.
In addition I have added two drive by fixes regarding move semantic and uninitialized variables.