Copy Constructor with STL Containers

Started by
10 comments, last by snk_kid 19 years, 6 months ago
Ok, guys, here's the problem. 1) I have a class (CSectorMap) that has two private STL Lists in them. Each list contains a pointer to a different object. 2) I have racked my brain trying to create a copy constructor for the class. Could someone please show me by example how to make one? Here is my code (minus the unrelated data) CSectorMap.h


#include "StdAfx.h"
#pragma warning(disable:4786)

typedef std::list<CMap*> CMapContainer;
typedef CMapContainer::iterator CMapIter;

typedef std::list<CStellarObject*> CSOContainer;
typedef CSOContainer::iterator CSOIter;

class CSectorMap
{
public:
CSectorMap();
CSectorMap (const CSectorMap &);// Copy Constructor
~CSectorMap();			// destructor
//CSectorMap & 	operator=(const CMap &);// = Assignment Operator


// List Functions for Maps and Stellar Objects
int	GetNumMaps() const { return Maps.size(); };
int	GetNumSOs() const { return SOs.size(); };
void	InsertMap(CMap & uMap);
void	DeleteAllMaps();
void	DeleteAllSOs();

private:
	CMapContainer	Maps;
	CSOContainer	SOs;
};	


And here is CSectorMap.cpp

CSectorMap::CSectorMap()
{
	CSectorMap::Maps.clear();
	CSectorMap::SOs.clear();
	CSectorMap::MapBegin = Maps.begin();
};

CSectorMap::~CSectorMap()
{
	// Deletes heap memory for maps and SOs
	CSectorMap::DeleteAllMaps();
	CSectorMap::DeleteAllSOs();
};
void CSectorMap::InsertMap(CMap &uMap)
{
	CMap *Temp = new CMap;
	*Temp = uMap;
	CSectorMap::Maps.push_back(Temp);
};

void CSectorMap::DeleteAllMaps()
{
	for (CMapIter CMapIT = Maps.begin(); CMapIT != Maps.end(); CMapIT++)
	{
		delete * CMapIT;
	};
	Maps.clear();
};

void CSectorMap::DeleteAllSOs()
{
	for (CSOIter CSOIT = SOs.begin(); CSOIT != SOs.end(); CSOIT++)
	{
		delete * CSOIT;
	};
	SOs.clear();
};

I removed my copy constructor because it didn't work anyway, and it was pretty messed up. Also, CMap and CStellarObject are not included, since the STL Containers only contains pointers to them. PRETTY PLEASE, someone show me how to make a copy constructor! Also, if you see anything wrong with my code (like a better way to do something), just let me know. Skywise [/source]
Advertisement
First of all why do you need to store pointers instead of the actual type in question? are they polymorphic types?
Because each CMap has a sizeof over 8000+ (And it uses pointers). I figured I would speed things up by searching the list containing 4-byte pointers than 8000+ byte objects.
Quote:Original post by TheSkywise
Because each CMap has a sizeof over 8000+ (And it uses pointers). I figured I would speed things up by searching the list containing 4-byte pointers than 8000+ byte objects.


The size of instance doesn't make searching an item in container any faster, it makes no difference.

How-ever you may actually benefit from storing pointers in a container because you have large objects and you don't wont the overhead of the copy-constructor when you add an item to the container. Currently thou in your insert method your not getting this benefit because your still copy data by calling the assigment operator.

To make your life easier i would suggest you use smart pointers instead such boost's smart pointers in particular shared_ptr, they will delete memory for you at the wright time.

I'll post you some code to show how to handle the copy constructor later, note also you don't have to have a copy constructor if it doesn't make sense to copy/assign, just make it private.
Thanks.

I understand the basic concept of smart pointers, but have never implemented them. Thanks for the link. I will look into it.

I really need to know how to make a copy constructor for this class. I have reread chapters on pointers, references, etc... I have looked in "Thinking in C++", I have checked the internet. I have searched the forum. I have nearly exhausted my means of research...

Your way of calling functions is unorthodox.

CSectorMap::Maps.clear();
CSectorMap::SOs.clear();
CSectorMap::MapBegin = Maps.begin();

just do

Maps.clear();
SOs.clear();
MapBegin = Maps.begin();//this member doesn't exist in your example

As that member doesn't exist it makes me think you're not showing us the full code, which is a bit of a waste of time.

Actually calling clear isn't needed as they are created empty in the first place.

If you store actual objects rather than pointers you won't need to call new at all. The containers will look after all memory concerns for you so you won't need to write a copy constructor or destructor either.
typedef std::list<CMap> CMapContainer;
typedef std::list<CStellarObject> CSOContainer;
to write a copy constructor and assignment operator for classes which contain pointers to internal objects (and also for ones that contain lists which contain pointers to objects) you need to ask yourself - What are the ownership rules for the objects you are pointing to?

* Who allocates them (who new'd them in the first place)
* Who owns them (who's job will it be to delete them)
...

For normal objects that are not ref counted, you only have 2 likely options:

1. some OTHER list is considered the "owner" of the objects pointed to, and your lists in question are just "pointing" to them ... no ownership at all, no deleting on distruction. For this case, you just copy the list of pointers, no special work ... and make sure your destructor just deletes the list NOT the actual objects ... make sure that the "owner" list eventually deletes the objects themselves ... and make sure the owner list doesn't delete the objects prior to the user lists boing deleted.

2. each list creates and owns the pointed to objects. In this case each time you insert an object you make a COPY of it (using new) and own the copy (delete it at destruction time). So in this case, your copy constructor would for loop through the list being copied and use new to copy each object ...

The other choice you have is to use some form of "smart pointer" or "ref counted object" system. In that case, you have a list of ref counted things (either implicitly ref counted, like a smart pointer - or explicitly ref counted, like COM objects).

In the smart pointer case, you just copy the smart pointer (which internally changes the ref count) ... in the ref counted object case ... you list would be a list of pointers to ojects .. and when you copy the pointers you AddRef() (whatever your ref count increment function is), and when you are destroyed you Release() (decrement ref count) each object ...

many choices for many situations.
Quote:
For normal objects that are not ref counted, you only have 2 likely options:

1. some OTHER list is considered the "owner" of the objects pointed to, and your lists in question are just "pointing" to them ... no ownership at all, no deleting on distruction. For this case, you just copy the list of pointers, no special work ... and make sure your destructor just deletes the list NOT the actual objects ... make sure that the "owner" list eventually deletes the objects themselves ... and make sure the owner list doesn't delete the objects prior to the user lists boing deleted.

2. each list creates and owns the pointed to objects. In this case each time you insert an object you make a COPY of it (using new) and own the copy (delete it at destruction time). So in this case, your copy constructor would for loop through the list being copied and use new to copy each object ...

The other choice you have is to use some form of "smart pointer" or "ref counted object" system. In that case, you have a list of ref counted things (either implicitly ref counted, like a smart pointer - or explicitly ref counted, like COM objects).

In the smart pointer case, you just copy the smart pointer (which internally changes the ref count) ... in the ref counted object case ... you list would be a list of pointers to ojects .. and when you copy the pointers you AddRef() (whatever your ref count increment function is), and when you are destroyed you Release() (decrement ref count) each object ...

many choices for many situations.


Can you show me a copy constructor with option 2 above using my code framework?
Quote:Original post by Xai
2. each list creates and owns the pointed to objects. In this case each time you insert an object you make a COPY of it (using new) and own the copy (delete it at destruction time). So in this case, your copy constructor would for loop through the list being copied and use new to copy each object ...


If you do that then there isn't much point of storing pointers because STL containers does something like this internally already, the exception to this is if you neededed a polymorphic behaviour or a heterogeneous container but at the same maintain copy schematics. If this isn't the case then your better off storing the type in question rather than a pointer to the type in question.

TheSkywise i would say you just store the actual type and only when you do really find an overhead from profiling then consider storing pointers to avoid copy construction from occuring each time an element is added to the container.
snk_kid you are right that the "own and make copies" method removes many of the benifits of using pointers in the first place ... but not all. For instance, if you are going to actually change the values of your lists objects and don't want to affect the original sources list ... you NEED copies. Of course, as you said, you could just store items instead of pointers then, but then what would you do if you had to sort or rearange your list a lot ...

all i'm sayin is that there are valid reasons for every possible type of storage and ownership combination you can think of ...

to the OP, I'm at work right now, but I'll try to post you an example when I get home tonight.

an off the top of my head quick and dirty snippet (I am leaving off obvious stuff like extra functions, typedefs, includes, and namespace qualifiers) not directly from your framework would be:

class Example  {  private:    typedef list<SomeType*> ListType;    ListType objects;  public:    Example(const Example &rhs);  };Example::Example(const Example &rhs)  {  objects.resize(rhs.size();  ListType::iterator obj_it = objects.begin();  foreach(ListType::iterator rhs_it = rhs.begin(); rhs_it != rhs.end(); ++rhs.it)    {    *obj_it = new SomeType(*(*rhs_it));    ++obj_it;    }  }


I know it's a little ugly ... I'm sure I could do better given 5 more minutes ... and I wish I had a c++ project set up to compile / test it ... but ...

good luck.

This topic is closed to new replies.

Advertisement