Commit 4df86bfa authored by Russell Taylor's avatar Russell Taylor
Browse files

Remove some Geometry memory leaks (including ones just in tests). Re #722.

parent 64cf1e3e
...@@ -53,7 +53,7 @@ namespace Geometry ...@@ -53,7 +53,7 @@ namespace Geometry
File change history is stored at: <https://svn.mantidproject.org/mantid/trunk/Code/Mantid>. File change history is stored at: <https://svn.mantidproject.org/mantid/trunk/Code/Mantid>.
Code Documentation is available at: <http://doxygen.mantidproject.org> Code Documentation is available at: <http://doxygen.mantidproject.org>
*/ */
class DLLExport Component:public virtual IComponent class DLLExport Component:public virtual IComponent
{ {
public: public:
/// Returns a string representation of the component type /// Returns a string representation of the component type
......
...@@ -25,17 +25,14 @@ namespace Geometry ...@@ -25,17 +25,14 @@ namespace Geometry
//---------------------------------------------------------------------- //----------------------------------------------------------------------
// Forward declaration // Forward declaration
//---------------------------------------------------------------------- //----------------------------------------------------------------------
class Parameter;
class Parameter; /** The ParameterFactory class creates parameters for the instrument ParameterMap.
/** The AlgorithmFactory class creates parameters for the instrument ParameterMap.
It inherits most of its implementation from the Dynamic Factory base class.
It is implemented as a singleton class.
@author Roman Tolchenov, Tessella plc @author Roman Tolchenov, Tessella plc
@date 19/05/2009 @date 19/05/2009
Copyright &copy; 2007 STFC Rutherford Appleton Laboratories Copyright &copy; 2009 STFC Rutherford Appleton Laboratories
This file is part of Mantid. This file is part of Mantid.
...@@ -53,76 +50,56 @@ namespace Geometry ...@@ -53,76 +50,56 @@ namespace Geometry
along with this program. If not, see <http://www.gnu.org/licenses/>. along with this program. If not, see <http://www.gnu.org/licenses/>.
File change history is stored at: <https://svn.mantidproject.org/mantid/trunk/Code/Mantid> File change history is stored at: <https://svn.mantidproject.org/mantid/trunk/Code/Mantid>
*/ */
//class DLLExport ParameterFactoryImpl : public Kernel::DynamicFactory<Parameter> class DLLExport ParameterFactory
// { {
// public: public:
// /// Creates a new instance of parameter of type className template<class C>
// Parameter* create(const std::string& className, const std::string& name) const; static void subscribe(const std::string& className);
// template<class C>
// void subscribe(const std::string& className); static boost::shared_ptr<Parameter> create(const std::string& className, const std::string& name);
// private:
// friend struct Mantid::Kernel::CreateUsingNew<ParameterFactoryImpl>; private:
// /// Private default constructor
// /// Private Constructor for singleton class ParameterFactory();
// ParameterFactoryImpl(): Kernel::DynamicFactory<Parameter>(), g_log(Kernel::Logger::get("ParameterFactory")){} /// Private copy constructor
// /// Private copy constructor - NO COPY ALLOWED ParameterFactory(const ParameterFactory&);
// ParameterFactoryImpl(const ParameterFactoryImpl&); /// Private assignment operator
// /// Private assignment operator - NO ASSIGNMENT ALLOWED ParameterFactory& operator=(const ParameterFactory&);
// ParameterFactoryImpl& operator = (const ParameterFactoryImpl&);
// ///Private Destructor /// A typedef for the instantiator
// virtual ~ParameterFactoryImpl(){} typedef Kernel::AbstractInstantiator<Parameter> AbstractFactory;
// ///static reference to the logger class /// An inner class to specialise map such that it does a deep delete when s_map is destroyed
// Kernel::Logger& g_log; class FactoryMap : public std::map<std::string, AbstractFactory*>
// {
// }; public:
// /// Destructor. Deletes the AbstractInstantiator objects stored in the map.
// ///Forward declaration of a specialisation of SingletonHolder for AlgorithmFactoryImpl (needed for dllexport/dllimport) and a typedef for it. virtual ~FactoryMap()
//#ifdef _WIN32 {
//// this breaks new namespace declaraion rules; need to find a better fix for (iterator it = this->begin(); it!=this->end(); ++it) delete it->second;
// template class DLLExport Mantid::Kernel::SingletonHolder<ParameterFactoryImpl>; }
//#endif /* _WIN32 */ };
// typedef DLLExport Mantid::Kernel::SingletonHolder<ParameterFactoryImpl> ParameterFactory; /// The map holding the registered class names and their instantiators
// static FactoryMap s_map;
};
class DLLExport ParameterFactory
{
public:
/// A typedef for the instantiator
typedef Kernel::AbstractInstantiator<Parameter> AbstractFactory;
/// A typedef for the map of registered classes
typedef std::map<std::string, AbstractFactory*> FactoryMap;
template<class C>
static void subscribe(const std::string& className);
static Parameter* create(const std::string& className, const std::string& name);
private:
/// Private default constructor
ParameterFactory();
/// Private copy constructor
ParameterFactory(const ParameterFactory&);
/// Private assignment operator
ParameterFactory& operator=(const ParameterFactory&);
/// The map holding the registered class names and their instantiators
static FactoryMap s_map;
};
/** Templated method for parameter subscription /** Templated method for parameter subscription
* @param className The parameter type name * @param className The parameter type name
* @tparam C The parameter type * @tparam C The parameter type
*/ */
template<class C> template<class C>
void ParameterFactory::subscribe(const std::string& className) void ParameterFactory::subscribe(const std::string& className)
{ {
typename FactoryMap::iterator it = s_map.find(className); typename FactoryMap::iterator it = s_map.find(className);
if (!className.empty() && it == s_map.end()) if (!className.empty() && it == s_map.end())
{ {
s_map[className] = new Kernel::Instantiator<C, Parameter>; s_map[className] = new Kernel::Instantiator<C, Parameter>;
} }
else else
{ {
throw std::runtime_error("Parameter type" + className + " is already registered.\n"); throw std::runtime_error("Parameter type" + className + " is already registered.\n");
} }
} }
} // namespace Geometry } // namespace Geometry
} // namespace Mantid } // namespace Mantid
......
...@@ -81,23 +81,24 @@ public: ...@@ -81,23 +81,24 @@ public:
/// Method for adding a parameter providing its value as a string /// Method for adding a parameter providing its value as a string
void add(const std::string& type,const IComponent* comp,const std::string& name, const std::string& value) void add(const std::string& type,const IComponent* comp,const std::string& name, const std::string& value)
{ {
Parameter* param = ParameterFactory::create(type,name); boost::shared_ptr<Parameter> param = ParameterFactory::create(type,name);
param->fromString(value); param->fromString(value);
m_map.insert(std::make_pair(comp,boost::shared_ptr<Parameter>(param))); m_map.insert(std::make_pair(comp,param));
} }
/// Method for adding a parameter providing its value of a particular type /// Method for adding a parameter providing its value of a particular type
template<class T> template<class T>
void add(const std::string& type,const IComponent* comp,const std::string& name, const T& value) void add(const std::string& type,const IComponent* comp,const std::string& name, const T& value)
{ {
ParameterType<T> *param = dynamic_cast<ParameterType<T> *>(ParameterFactory::create(type,name)); boost::shared_ptr<Parameter> param = ParameterFactory::create(type,name);
if (!param) ParameterType<T> *paramT = dynamic_cast<ParameterType<T> *>(param.get());
{ if (!paramT)
reportError("Error in adding parameter: incompatible types"); {
throw std::runtime_error("Error in adding parameter: incompatible types"); reportError("Error in adding parameter: incompatible types");
} throw std::runtime_error("Error in adding parameter: incompatible types");
param->setValue(value); }
m_map.insert(std::make_pair(comp,boost::shared_ptr<Parameter>(param))); paramT->setValue(value);
m_map.insert(std::make_pair(comp,param));
} }
/// Method is provided to add a parameter of any custom type which must be created with 'new' operator. /// Method is provided to add a parameter of any custom type which must be created with 'new' operator.
......
...@@ -46,8 +46,8 @@ namespace Mantid ...@@ -46,8 +46,8 @@ namespace Mantid
*/ */
CacheGeometryGenerator::~CacheGeometryGenerator() CacheGeometryGenerator::~CacheGeometryGenerator()
{ {
if(mFaces!=NULL) delete mFaces; if(mFaces!=NULL) delete[] mFaces;
if(mPoints!=NULL) delete mPoints; if(mPoints!=NULL) delete[] mPoints;
} }
...@@ -80,8 +80,8 @@ namespace Mantid ...@@ -80,8 +80,8 @@ namespace Mantid
*/ */
void CacheGeometryGenerator::setGeometryCache(int noPts,int noFaces,double* pts,int* faces) void CacheGeometryGenerator::setGeometryCache(int noPts,int noFaces,double* pts,int* faces)
{ {
if(mPoints!=NULL) delete mPoints; if(mPoints!=NULL) delete[] mPoints;
if(mFaces!=NULL) delete mFaces; if(mFaces!=NULL) delete[] mFaces;
mNoOfVertices=noPts; mNoOfVertices=noPts;
mNoOfTriangles=noFaces; mNoOfTriangles=noFaces;
mPoints=pts; mPoints=pts;
......
...@@ -308,9 +308,8 @@ namespace Mantid ...@@ -308,9 +308,8 @@ namespace Mantid
} }
} }
} }
#if debugObject
std::cerr<<"Populated surfaces =="<<Rcount<<" in "<<ObjName<<std::endl; PLog.debug() <<"Populated surfaces =="<<Rcount<<" in "<<ObjName<<std::endl;
#endif
createSurfaceList(); createSurfaceList();
return 0; return 0;
......
...@@ -7,7 +7,7 @@ namespace Mantid ...@@ -7,7 +7,7 @@ namespace Mantid
namespace Geometry namespace Geometry
{ {
/// @cond /// @cond
template<> template<>
void ParameterV3D::fromString(const std::string &value) void ParameterV3D::fromString(const std::string &value)
{ {
...@@ -16,29 +16,28 @@ namespace Geometry ...@@ -16,29 +16,28 @@ namespace Geometry
} }
/// @endcond /// @endcond
ParameterFactory::FactoryMap ParameterFactory::s_map; ParameterFactory::FactoryMap ParameterFactory::s_map;
/** Creates an instance of a parameter /** Creates an instance of a parameter
* @param className The parameter registered type name * @param className The parameter registered type name
* @param name The parameter name * @param name The parameter name
* @return A pointer to the created parameter * @return A pointer to the created parameter
* @throw runtime_error if the type has not been registered * @throw runtime_error if the type has not been registered
*/ */
Parameter* ParameterFactory::create(const std::string& className, const std::string& name) boost::shared_ptr<Parameter> ParameterFactory::create(const std::string& className, const std::string& name)
{ {
Parameter* p = NULL; boost::shared_ptr<Parameter> p;
FactoryMap::const_iterator it = s_map.find(className); FactoryMap::const_iterator it = s_map.find(className);
if (it != s_map.end()) if (it != s_map.end())
p = it->second->createUnwrappedInstance(); p = it->second->createInstance();
else else
throw std::runtime_error("ParameterFactory:"+ className + " is not registered.\n"); throw std::runtime_error("ParameterFactory:"+ className + " is not registered.\n");
p->m_name = name; p->m_name = name;
p->m_type = className; p->m_type = className;
return p; return p;
} }
} // Namespace Geometry } // Namespace Geometry
} // Namespace Mantid } // Namespace Mantid
DECLARE_PARAMETER(int,int) DECLARE_PARAMETER(int,int)
...@@ -47,4 +46,3 @@ DECLARE_PARAMETER(bool,bool) ...@@ -47,4 +46,3 @@ DECLARE_PARAMETER(bool,bool)
DECLARE_PARAMETER(str,std::string) DECLARE_PARAMETER(str,std::string)
DECLARE_PARAMETER(V3D,Mantid::Geometry::V3D) DECLARE_PARAMETER(V3D,Mantid::Geometry::V3D)
DECLARE_PARAMETER(Quat,Mantid::Geometry::Quat) DECLARE_PARAMETER(Quat,Mantid::Geometry::Quat)
...@@ -184,25 +184,27 @@ void ...@@ -184,25 +184,27 @@ void
Intersection::setLeaf(Rule* nR,const int side) Intersection::setLeaf(Rule* nR,const int side)
/*! /*!
Replaces a leaf with a rule. Replaces a leaf with a rule.
No deletion is carried out Calls delete on previous leaf.
\param nR :: new rule \param nR :: new rule
\param side :: side to use \param side :: side to use
- 0 == LHS - 0 == LHS
- true == RHS - 1 == RHS
*/ */
{ {
if (side) if (side)
{ {
B=nR; delete B;
if (B) B=nR;
B->setParent(this); if (B)
} B->setParent(this);
}
else else
{ {
A=nR; delete A;
if (A) A=nR;
A->setParent(this); if (A)
} A->setParent(this);
}
return; return;
} }
...@@ -478,25 +480,27 @@ void ...@@ -478,25 +480,27 @@ void
Union::setLeaf(Rule* nR,const int side) Union::setLeaf(Rule* nR,const int side)
/*! /*!
Replaces a leaf with a rule. Replaces a leaf with a rule.
No deletion is carried out Calls delete on previous leaf.
\param nR :: new rule \param nR :: new rule
\param side :: side to use \param side :: side to use
- 0 == LHS - 0 == LHS
- true == RHS - 1 == RHS
*/ */
{ {
if (side) if (side)
{ {
B=nR; delete B;
if (B) B=nR;
B->setParent(this); if (B)
} B->setParent(this);
}
else else
{ {
A=nR; delete A;
if (A) A=nR;
A->setParent(this); if (A)
} A->setParent(this);
}
return; return;
} }
...@@ -705,7 +709,7 @@ SurfPoint::SurfPoint() : Rule(), ...@@ -705,7 +709,7 @@ SurfPoint::SurfPoint() : Rule(),
{} {}
SurfPoint::SurfPoint(const SurfPoint& A) : Rule(), SurfPoint::SurfPoint(const SurfPoint& A) : Rule(),
key(A.key),keyN(A.keyN),sign(A.sign) key(A.key->clone()),keyN(A.keyN),sign(A.sign)
/*! /*!
Copy constructor Copy constructor
\param A ::SurfPoint to copy \param A ::SurfPoint to copy
...@@ -732,11 +736,12 @@ SurfPoint::operator=(const SurfPoint& A) ...@@ -732,11 +736,12 @@ SurfPoint::operator=(const SurfPoint& A)
*/ */
{ {
if (&A!=this) if (&A!=this)
{ {
key=A.key; delete key;
keyN=A.keyN; key=A.key->clone();
sign=A.sign; keyN=A.keyN;
} sign=A.sign;
}
return *this; return *this;
} }
...@@ -744,7 +749,9 @@ SurfPoint::~SurfPoint() ...@@ -744,7 +749,9 @@ SurfPoint::~SurfPoint()
/*! /*!
Destructor Destructor
*/ */
{} {
delete key;
}
void void
SurfPoint::setLeaf(Rule* nR,const int) SurfPoint::setLeaf(Rule* nR,const int)
...@@ -817,10 +824,11 @@ SurfPoint::setKeyN(const int Ky) ...@@ -817,10 +824,11 @@ SurfPoint::setKeyN(const int Ky)
void void
SurfPoint::setKey(Surface* Spoint) SurfPoint::setKey(Surface* Spoint)
/*! /*!
Sets the key pointter Sets the key pointer. The class takes ownership.
\param Spoint :: new key values \param Spoint :: new key values
*/ */
{ {
if (key!=Spoint) delete key;
key=Spoint; key=Spoint;
return; return;
} }
......
...@@ -411,45 +411,44 @@ Rule::removeItem(Rule* &TRule,const int SurfN) ...@@ -411,45 +411,44 @@ Rule::removeItem(Rule* &TRule,const int SurfN)
*/ */
{ {
int cnt(0); int cnt(0);
Rule* Ptr=TRule->findKey(SurfN); Rule* Ptr = TRule->findKey(SurfN);
while(Ptr) while (Ptr)
{ {
Rule* LevelOne=Ptr->getParent(); // Must work Rule* LevelOne = Ptr->getParent(); // Must work
Rule* LevelTwo=(LevelOne) ? LevelOne->getParent() : 0; Rule* LevelTwo = (LevelOne) ? LevelOne->getParent() : 0;
if (LevelTwo) /// Not the top level if (LevelTwo) /// Not the top level
{ {
// Decide which of the pairs is to be copied // Decide which of the pairs is to be copied
Rule* PObj= (LevelOne->leaf(0)!=Ptr) ? Rule* PObj = (LevelOne->leaf(0) != Ptr) ? LevelOne->leaf(0) : LevelOne->leaf(1);
LevelOne->leaf(0) : LevelOne->leaf(1); //
// LevelOne->setLeaves(0, 0); // Delete from Ptr, and copy
LevelOne->setLeaves(0,0); // Delete from Ptr, and copy const int side = (LevelTwo->leaf(0) != LevelOne) ? 1 : 0;
const int side=(LevelTwo->leaf(0)!=LevelOne) ? 1 : 0; LevelTwo->setLeaf(PObj, side);
LevelTwo->setLeaf(PObj,side); }
} else if (LevelOne) // LevelOne is the top rule
else if (LevelOne) // LevelOne is the top rule {
{ // Decide which of the pairs is to be copied
// Decide which of the pairs is to be copied Rule* PObj = (LevelOne->leaf(0) != Ptr) ? LevelOne->leaf(0) : LevelOne->leaf(1);
Rule* PObj= (LevelOne->leaf(0)!=Ptr) ?
LevelOne->leaf(0) : LevelOne->leaf(1); PObj->setParent(0); /// New Top rule
TRule = PObj;
PObj->setParent(0); /// New Top rule LevelOne->setLeaves(0, 0); // Delete from Ptr, and copy
TRule=PObj; // Note we now need to delete this
LevelOne->setLeaves(0,0); // Delete from Ptr, and copy
// Note we now need to delete this
}
else // Basic surf object
{
SurfPoint* SX=dynamic_cast<SurfPoint*>(Ptr);
SX->setKeyN(0);
SX->setKey(0);
return cnt+1;
}
delete Ptr;
delete LevelOne; delete LevelOne;
Ptr=TRule->findKey(SurfN);
cnt++;
} }
else // Basic surf object
{
SurfPoint* SX = dynamic_cast<SurfPoint*> (Ptr);
SX->setKeyN(0);
SX->setKey(0);
return cnt + 1;
}
delete Ptr;
//delete LevelOne; // Shouldn't delete now we're deleting in setLeaf.
Ptr = TRule->findKey(SurfN);
cnt++;
}
return cnt; return cnt;
} }
...@@ -627,13 +626,14 @@ Rule::substituteSurf(const int SurfN,const int newSurfN,Surface* SPtr) ...@@ -627,13 +626,14 @@ Rule::substituteSurf(const int SurfN,const int newSurfN,Surface* SPtr)
int cnt(0); int cnt(0);
SurfPoint* Ptr=dynamic_cast<SurfPoint*>(findKey(SurfN)); SurfPoint* Ptr=dynamic_cast<SurfPoint*>(findKey(SurfN));
while(Ptr) while(Ptr)
{ {
Ptr->setKeyN(Ptr->getSign()*newSurfN); Ptr->setKeyN(Ptr->getSign()*newSurfN);
Ptr->setKey(SPtr); Ptr->setKey(SPtr->clone()); // Clone to ensure uniqueness
cnt++; cnt++;
// get next key // get next key
Ptr=dynamic_cast<SurfPoint*>(findKey(SurfN)); Ptr=dynamic_cast<SurfPoint*>(findKey(SurfN));