Memory leaks using CTypedPtrArray

[DalTXColtsFan]

I'm getting memory leaks out the wazoo in this class I'm trying to write. I'm just reading data from a file into a CTypedPtrArray-derived class. Don't classed derived from this template automatically deallocate themselves? If so, what might I be doing wrong?

unsigned short nVertexCount;
myFile.Read(&nVertexCount, sizeof(unsigned short));
m_lstVertices.SetSize(nVertexCount);
int nIndex, nIndex2;
for (nIndex=0; nIndex<nVertexCount; nIndex++)
{
CMS3DVertex* myVertex;
m_lstVertices[nIndex] = new CMS3DVertex;
myVertex = m_lstVertices[nIndex];
myFile.Read(&myVertex->m_cFlags, sizeof(unsigned char));
myFile.Read(myVertex->m_afVertex, sizeof(float) * 3);
myFile.Read(&myVertex->m_cBoneID, sizeof(char));
myFile.Read(&myVertex->m_cRefCount, sizeof(unsigned char));
}

Anything jump out?

The other classes are loaded similarly. The entire code can be found in a link at the bottom of the page: http://daltxcoltsfan.tripod.com - to get it up and running, look in the Initialize() routine in main.cpp and set the path the the ms3d file correctly, then the cursor keys move around and stuff.

As always, help appreciated!

[souldog]

since you
m_lstVertices[nIndex] = new CMS3DVertex;
you need to delete that

Just curious why you are using pointers for this at all.

[DalTXColtsFan]

I couldn't figure out how to do it without pointers. I tried several permutations using CMS3DVertex instead of CMS3DVertex* and m_lstVertices.Add but I couldn't get *any* of them to work.

I had tried putting RemoveAll in the destructor of the CMilkshapeModel class but that didn't work either, so per your advice I did this:

int nIndex;
for (nIndex = 0; nIndex<m_lstVertices.GetSize(); nIndex++)
{
delete m_lstVertices[nIndex];
}

for (nIndex = 0; nIndex<m_lstMaterials.GetSize(); nIndex++)
{
delete m_lstMaterials[nIndex];
}

for (nIndex = 0; nIndex<m_lstMeshes.GetSize(); nIndex++)
{
delete m_lstMeshes[nIndex];
}

for (nIndex = 0; nIndex<m_lstTriangles.GetSize(); nIndex++)
{
delete m_lstTriangles[nIndex];
}

This took care of the memory leak, but is it "good code"? Is there a better way to get elements into a CTypedPtrArray?

Thanks as always for your help

Joe

[Paul McKenzie]

Originally posted by DalTXColtsFan
I couldn't figure out how to do it without pointers.

Which doesn't mean it can't be done. Show us what is CMS3DVertex, and I'm sure someone here will show you how to do this without pointers (provided that CMS3DVertex is safely copyable).

Once you do this without pointers, the rest of the problems will more than likely go away.

Regards,

Paul McKenzie

[DalTXColtsFan]

I'm a well-admitted non-expert, and if you guys want to suggest better ways to write code I'm all eyes.

Here's the CMS3DVertex class:

<code>
class CMS3DVertex : public CObject
{
public:
CMS3DVertex();
virtual ~CMS3DVertex();
unsigned char m_cFlags;
float m_afVertex[3];
char m_cBoneID;
unsigned char m_cRefCount;
CMS3DVertex operator=(CMS3DVertex &iobjVertex);
CMS3DVertex(CMS3DVertex &iobjVertex);
};
</code>

and here are the copy constructor and = operator codes:

<code>
CMS3DVertex CMS3DVertex:perator=(CMS3DVertex &iobjVertex)
{

m_cBoneID = iobjVertex.m_cBoneID;
m_cFlags = iobjVertex.m_cFlags;
m_cRefCount = iobjVertex.m_cRefCount;
m_afVertex[0] = iobjVertex.m_afVertex[0];
m_afVertex[1] = iobjVertex.m_afVertex[1];
m_afVertex[2] = iobjVertex.m_afVertex[2];

return *this;
}

CMS3DVertex::CMS3DVertex(CMS3DVertex &iobjVertex)
{

m_cBoneID = iobjVertex.m_cBoneID;
m_cFlags = iobjVertex.m_cFlags;
m_cRefCount = iobjVertex.m_cRefCount;
m_afVertex[0] = iobjVertex.m_afVertex[0];
m_afVertex[1] = iobjVertex.m_afVertex[1];
m_afVertex[2] = iobjVertex.m_afVertex[2];

}
</code>

The text file I'm reading from has the vertex count at the beginning. All I want to do is create a CTypedPtrArray of CMS3DVertex objects as each is read in from the file. I would LOVE to see how to do that without pointers!

Thanks a ton
Joe

[sstofka]

I didn't look at your code for your vertex class CMS3DVertex but it is derived from CObject so write a Serialize fxn for the class.

Then at the end of your Document's Serialize fxn
m_lstVertices.Serialize(ar);
m_lstMaterials.Serialize(ar);
m_lstTriangles.Serialize(ar);

It's a done deal!

Also, here's a macro for clearing memory in CTypedPtrArray

#define PTRARRAY_CLEAR(arrayname) \
{int i = 0; \
while (i < (arrayname).GetSize()) delete (arrayname).GetAt(i++); \
(arrayname).RemoveAll(); (arrayname).FreeExtra();}

So, if these arrays are members of your Document class and this is an SDI app, put this in DeleteContents(), instead of the destructor, so that memory will be cleared when the Document object is reused to open another file.

PTRARRAY_CLEAR(m_lstVertices)
PTRARRAY_CLEAR(m_lstMaterials)
PTRARRAY_CLEAR(m_lstTriangles)

Steve

[DalTXColtsFan]

Thanks ssoftka

I don't think I can use Serialize without customizing the snot out of it, because the files I'm loading are Milkshape 3D files (www.milkshape3d.com), and when I export the files, I'm going to export them in my own format that's kind of a conglomeration of everything else in memory.

Thanks for the code on how to clear out the array.

Do you happen to know if it's typically customary to clear out allocated memory in a CTypedPtrArray yourself? I was expecting the deconstructor to handle that but I guess I was wrong.

Thanks!
Joe

[TheCPUWizard]

A CTypedPtrArray does not "own" the object. If the object is not an RCO, then you will have a memory leak.

If you want to use MFC classes, and want to use pointers, then consider developing your owned template for a COwnedTypedPtrArray. But be careful, if you clear the array and there are any other references to the objects, they will become dangling.


sstofka's solution works but has IMHO some disadvatages

1) It has the same problem of leaving dangling references
2) It requires an explicit user actuion to avoid the leak.
3) It is a macro, and macros "should" be avoided.

The best solution would be to make all of the items derive from some RCO base class and not have to worry about it, but this may not be practical if you already have a significant amount of code.

[souldog]

Originally posted by sstofka
#define PTRARRAY_CLEAR(arrayname) \
{int i = 0; \
while (i < (arrayname).GetSize()) delete (arrayname).GetAt(i++); \
(arrayname).RemoveAll(); (arrayname).FreeExtra();}
Steve

MACRO for this? How about a template funciton

[souldog]

class CMS3DVertex
{
public:
	CMS3DVertex():
	m_cFlags(0),
	m_cBoneID(0),
	m_cRefCount(0)
	{
		m_afVertex[0] = 0.0;
		m_afVertex[1] = 0.0;
		m_afVertex[2] = 0.0;
	}

	void Store(std::ostream& os) const
	{
		is.write(reinterpret_cast<const char*>(&m_cFlags), sizeof(unsigned char));
		is.write(reinterpret_cast<const char*>(&m_afVertex[0]), 3*sizeof(float));
		is.write(reinterpret_cast<const char*>(&m_cBoneID), sizeof(char));
		is.write(reinterpret_cast<const char*>(&m_cRefCount), sizeof(unsigned char));
	}

	void Load(std::istream& is)
	{
		is.read(reinterpret_cast<char*>(&m_cFlags), sizeof(unsigned char));
		is.read(reinterpret_cast<char*>(&m_afVertex[0]), 3*sizeof(float));
		is.read(reinterpret_cast<char*>(&m_cBoneID), sizeof(char));
		is.read(reinterpret_cast<char*>(&m_cRefCount), sizeof(unsigned char));
	}

	unsigned char m_cFlags;
	float m_afVertex[3];
	char m_cBoneID;
	unsigned char m_cRefCount;

};

__inline std::istream& operator>>(std::istream& is, CMS3DVertex vertex)
{
	vertex.Load(is);
	return is;
}

__inline std::ostream& operator<<(std::ostream& is, const CMS3DVertex vertex)
{
	vertex.Store(os);
	return os;
}


template<typename T>
void Store_ClassVector(const std::vector<T>& Tvector, std::ostream& os)
{
	std::vector<T>::size_type size(Tvector.size());
	os.write(reinterpret_cast<const char*>(&size), sizeof(std::vector<T>::size_type)); 	
	for(std::vector<T>::size_type i =0; i < size; ++i)
	{
		Tvector[i].Store(os);
	}
};

template<typename T>
void Load_ClassVector(std::vector<T>& Tvector, std::istream& is)
{
	std::vector<T>::size_type to_load_size(0);
	is.read(reinterpret_cast<char*>(&to_load_size), sizeof(std::vector<T>::size_type));
	Tvector.resize(to_load_size);
	for(std::vector<T>::size_type i =0; i < to_load_size; ++i)
	{
		Tvector[i].Load(is);
	}
};

//GET THE PATH FROM WHICH THE EXECUTALE IS RUNNING
__inline bool GetExcPath(std::string& ExcPath)  
{
	//Get the directory this app is executing from
	char szAppPath[MAX_PATH];
	::ZeroMemory(&szAppPath, MAX_PATH);
	if (!::GetModuleFileName(NULL, szAppPath, sizeof(szAppPath)-1))
		return false;
	ExcPath = szAppPath;
	return true;
}

//SOME FUNCTIONS FOR LOADING AND STORING TO A FILE 
template<typename T>
bool LoadBinaryFile(T& to_load, const std::string& file_name)
{
	std::string path("");
	if(!GetExcPath(path))
		return false;
	
	path = path.substr(0, path.rfind('\\')) + "\\" + file_name;

	//check to see if the file exists, if it does not return 0 to use defaults
	// if it does load values
	std::ifstream in_file(path.c_str(), std::ios::in | std::ios::binary);
	if(!in_file.is_open())
		return false;

	in_file >> to_load;
	
	return true;
};

template<typename T>
bool SaveBinaryFile(const T& to_save, const std::string& file_name) 
{
	std::string path("");
	if(!GetExcPath(path))
		return false;
	
	path = path.substr(0, path.rfind('\\')) + "\\" + file_name;

	std::ofstream out_file(path.c_str(), std::ios::out|std::ios::binary|std::ios::trunc);
	if(!out_file.is_open())
		return false;
	out_file << to_save;
	return true;
};

class Holds3dVertices
{
public:
	void Store(std::ostream& os) const
	{
		Store_ClassVector(Vertices_, os);
	}

	void Load(std::istream& is)
	{
		Load_ClassVector(Vertices_, is);
	}

	bool  LoadVerticesFromFile(const std::string file_name)
	{
		try
		{
			return LoadBinaryFile(*this, file_name);
		}
		catch(const std::exception& e)
		{
			::LogMessage(NULL, e.what, "ERROR", MB_ICONEXCLAMATION);
			return false;
			//handle error
		}
	}
	
	std::vector<CMS3DVertex> Vertices_;
};

__inline std::istream& operator>>(std::istream& is, Holds3dVertices vertices)
{
	vertices.Load(is);
	return is;
}

__inline std::ostream& operator<<(std::ostream& is, const Holds3dVertices vertices)
{
	vertices.Store(os);
	return os;
}

just typed it out so I have not tested this

[souldog]

Also since CObject is gone you can just use a struct and simplify
the code is you want

struct CMS3DVertex
{
	unsigned char m_cFlags;
	float m_afVertex[3];
	char m_cBoneID;
	unsigned char m_cRefCount;
};

template<typename T>
void Store_PODVector(const std::vector<T>& Tvector, std::ostream& os)
{
	std::vector<T>::size_type size(Tvector.size());
	os.write(reinterpret_cast<const char*>(&size), sizeof(std::vector<T>::size_type)); 	
	if(0 != size
		os.write(reinterpret_cast<const char*>(&Tvector[i]), size * sizeof(T));
};	

template<typename T>
void Load_PODVector(std::vector<T>& Tvector, std::istream& is)
{
	std::vector<T>::size_type to_load_size(0);
	is.read(reinterpret_cast<char*>(&to_load_size), sizeof(std::vector<T>::size_type));
	if(0 != to_load_size)
	{
		Tvector.resize(to_load_size);
		is.read(reinterpret_cast<char*>(&Tvector[0]), to_load_size * sizeof(T));
	}
};	

class Holds3dVertices
{
public:
	void Store(std::ostream& os) const{Store_PODVector(Vertices_, os);}

	void Load(std::istream& is){Load_PODVector(Vertices_, is);}

	bool  LoadVerticesFromFile(const std::string file_name)
	{
		try
		{
			return LoadBinaryFile(*this, file_name);
		}
		catch(const std::exception& e)
		{
			::LogMessage(NULL, e.what, "ERROR", MB_ICONEXCLAMATION);
			return false;
			//handle error
		}
	}
	
	std::vector<CMS3DVertex> Vertices_;
};

__inline std::istream& operator>>(std::istream& is, Holds3dVertices vertices)
{
	vertices.Load(is);
	return is;
}

__inline std::ostream& operator<<(std::ostream& is, const Holds3dVertices vertices)
{
	vertices.Store(os);
	return os;
}

[DalTXColtsFan]

wow, souldog - that gives me a lot to chew on! (no pun intended)

I'm going to have to research template classes and that vector struct - I've never used them before. When I do I'll get back to this.

Thanks again
Joe

[Paul McKenzie]

Originally posted by DalTXColtsFan
wow, souldog - that gives me a lot to chew on! (no pun intended)

I'm going to have to research template classes and that vector struct - I've never used them before. When I do I'll get back to this.

Thanks again
Joe

The std::vector is one of the standard C++ classes. It basically is a replacement for CArray (and CTypedPtrArray). It does not suffer from the same restrictions as CTypedPtrArray in that

1) You can use it for MFC, non-MFC, Windows and non-Windows programs (as a matter of fact, every ANSI standard C++ compiler should support it since it is an ANSI C++ standard class).

2) It does not need you to derive from CObject or any other such class. The requirements are only that your class is copyable and assignable, and your CMS3DVertex class is both safely copyable and assignable.

Regards,

Paul McKenzie