:Fixed OpenSim::Controller leaking memory whenever it is copied

- Happens because the OpenSim::Set<const Actuator> that it owns will automatically copy all elements, even if they are not owned
opensim-org · Jun 30, 2022 · bd8b11b · bd8b11b
1 parent 959fc82
commit bd8b11b
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 22 deletions.
diff --git a/OpenSim/Simulation/Control/Controller.cpp b/OpenSim/Simulation/Control/Controller.cpp
@@ -44,29 +44,47 @@ using namespace std;
 /**
  * Default constructor.
  */
-Controller::Controller() 
+Controller::Controller() :
+    ModelComponent{},
+    _numControls{0},
+    _actuatorSet{}
 {
-    constructProperties();
+    setAuthors("Ajay Seth, Frank Anderson, Chand John, Samuel Hamner");
+    constructProperty_enabled(true);
+    constructProperty_actuator_list();
+    _actuatorSet.setMemoryOwner(false);
 }
 
+Controller::Controller(Controller const& src) :
+    ModelComponent{src},
+    PropertyIndex_enabled{src.PropertyIndex_enabled},
+    PropertyIndex_actuator_list{src.PropertyIndex_actuator_list},
+    _numControls{src._numControls},
+    _actuatorSet{}
+{
+    _actuatorSet.setMemoryOwner(false);
+}
+
+Controller& Controller::operator=(Controller const& src)
+{
+    if (&src != this)
+    {
+        static_cast<ModelComponent&>(*this) = static_cast<ModelComponent const&>(src);
+        PropertyIndex_enabled = src.PropertyIndex_enabled;
+        PropertyIndex_actuator_list = src.PropertyIndex_actuator_list;
+        _numControls = src._numControls;
+        _actuatorSet.setSize(0);
+    }
+    return *this;
+}
+
+Controller::~Controller() noexcept = default;
 
 //=============================================================================
 // CONSTRUCTION
 //=============================================================================
 //_____________________________________________________________________________
 //_____________________________________________________________________________
-/**
- * Connect properties to local pointers.
- */
-void Controller::constructProperties()
-{
-    setAuthors("Ajay Seth, Frank Anderson, Chand John, Samuel Hamner");
-    constructProperty_enabled(true);
-    constructProperty_actuator_list();
-
-    // Set is only a reference list, not ownership
-    _actuatorSet.setMemoryOwner(false);
-}
 
 void Controller::updateFromXMLNode(SimTK::Xml::Element& node,
                                    int versionNumber) {
@@ -125,8 +143,8 @@ void Controller::extendConnectToModel(Model& model)
     // if we use a list Socket<Actuator> 
 
     // make sure controller does not take ownership
-    _actuatorSet.setMemoryOwner(false);
     _actuatorSet.setSize(0);
+    _actuatorSet.setMemoryOwner(false);
 
     int nac = getProperty_actuator_list().size();
     if (nac == 0)
@@ -172,9 +190,9 @@ void Controller::setActuators(const Set<Actuator>& actuators)
     //TODO this needs to be setting a Socket list of Actuators
 
     // make sure controller does NOT assume ownership
+    _actuatorSet.setSize(0);
     _actuatorSet.setMemoryOwner(false);
     //Rebuild consistent set of actuator lists
-    _actuatorSet.setSize(0);
     updProperty_actuator_list().clear();
     for (int i = 0; i< actuators.getSize(); i++){
         addActuator(actuators[i]);

diff --git a/OpenSim/Simulation/Control/Controller.h b/OpenSim/Simulation/Control/Controller.h
@@ -81,6 +81,9 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Controller, ModelComponent);
 
     /** Default constructor. */
     Controller();
+    Controller(Controller const&);
+    Controller& operator=(Controller const&);
+    ~Controller() noexcept override;
 
     // Uses default (compiler-generated) destructor, copy constructor and copy 
     // assignment operator.
@@ -143,12 +146,6 @@ OpenSim_DECLARE_ABSTRACT_OBJECT(Controller, ModelComponent);
     // the (sub)set of Model actuators that this controller controls */ 
     Set<const Actuator> _actuatorSet;
 
-    // construct and initialize properties
-    void constructProperties();
-
-    //friend class ControlSet;
-    friend class ControllerSet;
-
 //=============================================================================
 };  // END of class Controller