Sign in to follow this  

My container classes

This topic is 4836 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Ok, I'm working on my game engine and I need a few container classes that are going to plug straight into it. The only two I have done are CResizeableArray and CString. I just wanted to see if I was missing any obvious memory leaks before I start using them. Please don't post saying I should use STL. I already know this and I'm using it everywhere else in my engine, just not the few places that these classes are designed specifically for (no arguing with me either, I'm using these wether you like it or not :D). Note that there are a few other functions to the classes that I have omitted for readability (I'm positive that there aren't any leaks in these).
template<class ContainerType>
class CResizeableArray
{
protected:
	ContainerType*	m_pArray;
	int				m_iArraySize;
	
public:
	CResizeableArray<ContainerType>(int iDefaultSize = 0)
	{
		m_pArray		= NULL;
		m_iArraySize	= 0;
		
		if(iDefaultSize > 1)
			ResizeArray(iDefaultSize);
	}
	
	
	~CResizeableArray<ContainerType>()
	{
		delete [] m_pArray;
	}
	
	
	ContainerType* ResizeArray(int iSize)
	{
		if(iSize > 1)
		{
			ContainerType* pBuffer = NULL;
			if(!(pBuffer = new ContainerType[iSize + m_iArraySize]))
				return NULL;
			
			if(m_pArray != NULL)
			{
				memcpy(pBuffer, m_pArray, m_iArraySize * sizeof(ContainerType));
				delete [] m_pArray;
			}
			
			m_pArray = pBuffer;
			m_iArraySize += iSize;
			
			return m_pArray;
		}
		
		return NULL;
	}
	
	
	void ClearArray()
	{
		delete [] m_pArray;
		m_iArraySize = 0;
	}
	
	
	ContainerType* GetArray() const
	{
		return m_pArray;
	}
	
	
	ContainerType GetArrayIndex(int iIndex) const
	{
		if(iIndex > -1 && iIndex < m_iArraySize)
			return m_pArray[iIndex];
	}
	
	
	ContainerType operator [](int iIndex) const
	{
		if(iIndex > -1 && iIndex < m_iArraySize)
			return m_pArray[iIndex];
		return NULL;
	}
	
	
	int GetSize() const
	{
		return m_iArraySize;
	}
};

class CString : public CResizeableArray<char>
{
public:
	CString()
	{
	}

	CString(char* pString)
	{
		ClearArray();
		ResizeArray(strlen(pString) + 1);
		strcpy(m_pArray, pString);
	}

	const CString& operator =(char* pString)
	{
		ClearArray();
		ResizeArray(strlen(pString) + 1);
		strcpy(m_pArray, pString);

		return *this;
	}

	const CString& operator =(string String)
	{
		ClearArray();
		ResizeArray(String.size());
		strcpy(m_pArray, String.c_str());

		return *this;
	}

	operator char*()
	{
		return GetArray();
	}

	char* GetString()
	{
		return GetArray();
	}
};


Thanks! [Edited by - Programmer16 on September 18, 2004 12:47:01 AM]

Share this post


Link to post
Share on other sites
The memcpy you are using to copy your arrays when resizing is not a good idea if you plan on storing anything complicated. For example, if you store a bunch of objects that require a deep copy thus have overloaded the assignment and copy constructors they will not get called. If you plan to store only pointers in your dynamic arrays then you should be fine but I would check out the STL vector source or other dynamic array implementations. I guarentee you those memcpys will cause problems for you.

Share this post


Link to post
Share on other sites
Protected member variables are a code smell.

Your string class shouldn't hit the heap for smaller strings.

CClassname is redundant. (FWIW)

A string isn't necessarily is-a type of resizable array. It should aggregate the resizable array.

Don't use strcpy for crying out loud! MSDN clearly states that incorrect usage of strcpy can trigger a buffer overflow!

The ResizeArray() member function doesn't make sense on a string.

Silent conversion operators can lead to ambiguities.

Use STL. If you dislike the notation, wrap the STL classes in your own and use those. But the use of strcpy() makes me question whether you should be using your own string class at this point, otherwise you're potentially introducing buffer overflows into your application when you get bad input.

Share this post


Link to post
Share on other sites
pammat: Actually, the STL does the same thing. Calling the copy constructor followed by the destructor for the original object is not as efficient as memcpy, and in practice doesn't usually cause problems that aren't obvious.

Share this post


Link to post
Share on other sites
@ Sneftel:
Quote:
Line from the constructor
if(iDefaultSize > 1)
ResizeArray(iDefaultSize);


@ antareus:
For some reason, I don't understand what this sentence means:
"A string isn't necessarily is-a type of resizable array. It
should aggregate the resizable array."

Do you mean it is a resizeable array or it isn't?
And (if I'm right in what aggregate means) you think that the CString class and CResizeable class should be the same?

I use the protected keyword to keep the data away from the users, but available to derived classes.

I know about the strcpy function, and I've been working on my own version of it to fix the problem.

I don't care if you don't like CClassname, but it's not redundant, it stands for Container :P. (What does FWIW mean?)

The ResizeArray() function can be used in the append function I'm going to add to it.

I already said that I'm using STL everywhere else, but I needed some extra functions and such added to it. First, I don't know how to derive from STL (I assume that it's probably like deriving from any other classes, but I don't know if I have to handle calls to certain behind the scene functions (like calling addref() in COM)). Second, did you guy's skip over the paragraph that says that I'm not going to use STL? I know STL is a better choice and all, but I'm still going to use these.

@ pammatt: I'm going to be using the CString class with my font, scripting, and console class. The CResizeableArray class is going to be used in part of my graphics system.

Share this post


Link to post
Share on other sites
Your string class should hold a copy of a resizable array inside of it to use as the buffer. It should not derive from the resizable array class. When you derive from a class you are saying "a string IS-A resizable array in the same way that a lion IS-AN animal." If I declare a string, it doesn't make any sense for me to be able to call a member function named "ResizeArray()" on a string. That is unnecessarily exposing implementation details. Private inheritance would be more appropriate in this situation.

I recommended you use STL because your use of strcpy() is questionable, as well as boundary conditions (allocating an array of one element) being handled and wrong delete operator being used. You wouldn't derive from an STL class but instead just have your class contain the STL container and then you'd implement whatever interface you want over it. I can understand your frustration with the external interface presented by STL - it is definitely pretty low-level and cryptic at times. But it is also tested, speedy code.

Share this post


Link to post
Share on other sites
Here, I've rewritten your array class for you. Rewriting the string class is left as an exercise to the reader.


template<class ContainerType>
class CResizeableArray
{
private:
std::vector<ContainerType> v;

public:
CResizeableArray<ContainerType>(int iDefaultSize = 0) :
v(iDefaultSize)
{ }


ContainerType* ResizeArray(int iSize)
{
v.resize(iSize);
}

void ClearArray()
{
v.clear();
}

ContainerType* GetArray() const
{
return v.empty() ? 0 : &v[0];
}

ContainerType GetArrayIndex(int iIndex) const
{
return v.at(iIndex);
}


ContainerType operator [](int iIndex) const
{
return v.at(iIndex);
}


int GetSize() const
{
return v.size();
}
};


Share this post


Link to post
Share on other sites
Quote:
Original post by pammatt
Really?! I could have sworn I saw many calls to construct, destory and copy objects around.. hmm.. guess you learn something new everyday!


Really. This is why im creating my own clone of the entire library (with some improvements IMHO). Here's my current base definition of ::industry::traits< ... >:

	template < typename T > class traits
{
public:
typedef magic::false_t trivial_ctor; //Constructor( no arguments )... 0ed memory OK if true_t.
typedef magic::false_t trivial_dtor; //Destructor... none exists to call if true_t.
typedef magic::false_t trivial_copy; //Copy Constructor( argument: const T & )... direct bitwise copy OK if true_t.
typedef magic::false_t trivial_move;
//Movement( done by memory managers... aka, vector realloc... )... okay to move purely
//bitwise if true_t. (seperate from trivial_copy, as for example, reference counting would
//stay the same in the case of handles... so there's really no need for some stuff in moves
//whereas it is needed for copys.
//typedef magic::false_t trivial_eval;
//Assignment( operator= ), no reason for existance yet noticed.
};


I might make trivial_move default to true_t... we'll see later on 0o.

edited to unbreak forum.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel
Here, I've rewritten your array class for you. Rewriting the string class is left as an exercise to the reader.

*** Source Snippet Removed ***


With this code though, I can use the return value from ResizeArray() unless I use the vector. I want to be able to directly interact with the data values:

CResizeableArray<Vertex3> MeshVertices(0);
int iInteger = 0;
// Named iInteger because I use it more than once
ifstream fin("model.txt");
fin>>iInteger;
Vertex3* pVertices = MeshVertices.ResizeArray(iInteger);

for(int iIndex = 0; iIndex < iInteger; ++iIndex)
{
float fX, fY, fZ;
float fNX, fNY, fNZ;
float fU, fV;

fin>>fX>>fY>>fZ;
fin>>fNX>>fNY>>fNZ;
fin>>fU>>fV;
pVertices[iIndex].m_Position = D3DXVECTOR3(fX, fY, fZ);
pVertices[iIndex].m_Normal = D3DXVECTOR3(fNX, fNY, fNZ);
pVertices[iIndex].m_TextureCoordinates = D3DXVECTOR2(fU, fV);
}




@ antareus - Ok, now I got it. I figured that out once I came to the problem of needing two resizeable arrays in a class.

Thanks

PS: As an added note, I had only worked on these for about an hour, so I wasn't worrying about things like strcpy() and single size creation. I've fixed the single size creation problem and I'm working on making a string copying function.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel

ContainerType GetArrayIndex(int iIndex) const
{
return v.at(iIndex);
}


ContainerType operator [](int iIndex) const
{
return v.at(iIndex);
}



I'd be concerned about these two first you'll end up with abit of redudndant copying, secondly using integers for index access not good idea better change it to:


template< typename ContainerType >
class CResizeableArray {
public:
typedef std::vector<ContainerType> vec;
private:
vec v;
public:
/* .... */
const ContainerType& GetArrayIndex(typename vec::size_type iIndex) const {
return v.at(iIndex);
}

ContainerType& GetArrayIndex(typename vec::size_type iIndex) {
return v.at(iIndex);
}

const ContainerType& operator[](typename vec::size_type iIndex) const {
return v[iIndex];
}

ContainerType& operator[](typename vec::size_type iIndex) {
return v[iIndex];
}
/* .... */
};

Share this post


Link to post
Share on other sites
Here is the finished version of the class, which takes use of STL and variable argument lists. I wasn't using STL before since my course at GameInstitute used code like it for a lesson or two. Now that I'm done with those, I can use STL :D.

Please note that with the variable arguments, if you are using a class then you MUST use a copy constructor when assigning them using the Containers::Vector::Vector() or Containers::Vector::Push() method (like I did with MyVector2 and std::string() in the source code below).


#include <windows.h>
#include <string>
#include <iostream>
#include <vector>
using namespace std;

namespace Containers
{
template <class ArrayType> class Vector
{
vector<ArrayType> m_Vector;

public:
Vector(int iDefaultSize, ...)
{
va_list VaList;
va_start(VaList, iDefaultSize);

for(int iIndex = 0; iIndex < iDefaultSize; ++iIndex)
{
m_Vector.push_back((ArrayType)va_arg(VaList, ArrayType));
}

va_end(VaList);
}

Vector()
{
}

void Push(int iCount, ...)
{
va_list VaList;
va_start(VaList, iDefaultSize);

for(int iIndex = 0; iIndex < iDefaultSize; ++iIndex)
{
m_Vector.push_back((ArrayType)va_arg(VaList, ArrayType));
}

va_end(VaList);
}

void Pop(int iCount)
{
for(int iIndex = 0; iIndex < m_Vector.size(); ++iIndex)
{
cout<<"Removing..."<<endl;
m_Vector.pop_back();
}
}

ArrayType operator[](int iIndex) const
{
return m_Vector[iIndex];
}

int Size() const
{
return m_Vector.size();
}
};
}

class Vertex
{
public:
float x, y, z;

Vertex(float fx, float fy, float fz)
{
Set(fx, fy, fz);
}

void Set(float fx, float fy, float fz)
{
x = fx;
y = fy;
z = fz;
}

void Print()
{
cout<<"("<<x<<","<<y<<","<<z<<")"<<endl;
}
};

typedef Containers::Vector<Vertex> VVertex;
typedef Containers::Vector<string> VString;

int main()
{
VVertex MyVector1(3, Vertex(10, 1, 1), Vertex(3, 1, 2), Vertex(2, -1, 31));
VString MyVector2(3, string("test123"), string("test321"), string("last test"));

for(int iIndex = 0; iIndex < MyVector1.Size(); ++iIndex)
{
MyVector1[iIndex].Print();
}

for(iIndex = 0; iIndex < MyVector2.Size(); ++iIndex)
{
cout<<MyVector2[iIndex].c_str()<<endl;
}
return 0;
}



Thanks!

Share this post


Link to post
Share on other sites
the use of va_list and classes just SCREAMS bad ju-ju in my mind - especially when you're passing them needlessly by value. If it's because you're too lazy to write insert(...) every time, either create an array of these and call:

template <class InputIterator>
void insert(iterator pos,
InputIterator f, InputIterator l)


like so:

vector< int > my_vector;

int my_ints[] = { 3 , 4 , 5 };
my_vector.insert( my_ints , my_ints + sizeof( my_ints )/sizeof( *my_ints ) );


or (if for some reason you just GOTTA have your own custom allocator) reimplement your class to use operator<< for inserting, so you can create code like this:

my_vector.insert()
<< MyClass( 3 )
<< MyClass( "pizza" , 5 )
<< MyClass( "etc" );


or even better yet one with an end_insert, with the operator<<'s creating an "insert list" so that my_vector can resize it's memory before inserting all of them, instead of possibly reallocating it's memory multiple times if it has less elements than it's being assigned. Since they're passing by const reference, you'll eliminate the overhead used up copying the class over and over.

Share this post


Link to post
Share on other sites
Hi!

I'd strongly recommend you don't resize your string array this way every time you use assignment operator - it's highly inefficient.

Instead increase your array's size by consecutive powers of 2.

So instead of using your:
ResizeArray(strlen(pString) + 1);

do the same with:
assureSize(strlen(pString) + 1);

Here a snippet from string class we use:

// Assures the size of the string to the specified value.
cVoid cString::assureSize(cInt minSize)
{
if (minSize <= Size)
return;

cInt size = Size > 0 ? Size * 2 : 1;
while (size < minSize)
size *= 2;

cChar* text = new cChar[size];
for (cInt i = 0; i < Length; i++)
text[i] = Text[i];

if (Text)
delete[] Text;
Text = text;
Size = size;
}


Cheers!

Share this post


Link to post
Share on other sites
Also:

Quote:
Originally posted by Fruny in the thread: General Programming - Unknown number of starting variables?
1) No, varargs do not typecheck. And passing an object (as opposed to a pointer to an object) like std::string has undefined behaviour [with varargs], which is standardese for "don't do this".


Aka, your container will likely fail on some compilers due to the use of varargs (giving clout to the screaming of bad ju-ju in my above post).

edit: also, see washu's post in that same thread where he makes this legal:

std::vector< int > v;
f((v, 10, 11, 12, 14));


in a typesafe, non-variable-argument manner, in a way that would allow you to pass by value (or reference which the standard variable-arguments system can not handle) by overloading operator, .

[Edited by - MaulingMonkey on September 20, 2004 12:41:13 AM]

Share this post


Link to post
Share on other sites

This topic is 4836 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this