Copy constructor and assignment operator for Container Class

[ljin]

I am trying to construct a template class Set<T> which models a container like the mathematical notion of a set. I should really provide an assignment operator and copy constructor for my set class. Here is piece of my code for Set<T> container:

template <class T>
class Set {
public:
Set(int initial_size = 4); //default constructor, allocates appropriate heap storage
//copy constructor
Set(const Set<T> & s):numberOfElements(s.numberOfElements),capacity(s.capacity),index(s.index){
elements = new T*[capacity];
T** temp = s.elements;
for(int i=0;i<numberOfElements;i++){
*elements[i]=*temp[i]; // Here is the problem
}
delete [] temp;
}
~Set(void); //destructor

//assignment operator
Set<T> & operator=(const Set<T> & s){
if(this != &s){
cout << "Set assignement operator" << "\n";

delete [] elements;

capacity = s.capacity;
numberOfElements = s.numberOfElements;
index = s.index;

elements = new T*[capacity];
T** temp = s.elements;
for(int i=0;i<index;i++){
*elements[i]=*temp[i]; // Here is the problem
}
delete [] temp;
}
return *this;
}

.....

private:
T ** elements; //to store elements (like assignment #1)
int numberOfElements; //number of elements in the set
int capacity; //max. no of elements the set can current memory can hold
mutable int index; //private variable used for the iteration
};


I already constructed a class Person which provide a copy constructor and an assignment operator. I will place People objects in the Set containers. Here is piece of Person class:

// The copy-constructor:
Person(const Person& p):birthday(p.birthday){
// cout << "Person copy constructor" << "\n";
char *temp = new char[strlen(p.name) + 1];
strncpy(temp, p.name, strlen(p.name) + 1);
name = temp;

char *tmp = new char[strlen(p.email_address)+1];
strncpy(tmp, p.email_address, strlen(p.email_address)+1);
email_address = tmp;
}
//Person assignement operator
Person & operator=(const Person & p){
if(&p != this){
cout << "Person assignement operator" << "\n";

delete [] name;
delete [] email_address;

char *temp = new char[strlen(p.name) + 1];
strncpy(temp, p.name, strlen(p.name) + 1);
name = temp;

char *tmp = new char[strlen(p.email_address)+1];
strncpy(tmp, p.email_address, strlen(p.email_address)+1);
email_address = tmp;


birthday = p.birthday;
}
return *this;
}


The copy constructor and assignment operator of the class Person work fine when I create the objects of the Person and do as follow:

Person *p1 = new Person("Lou", "[e-mail]", 20, 6, 1960);
Person p2 = p1;
Person p3;

p3=p1;
......

However, when I placed many People objects in the Set containers and made copy and assignment of the Set container, the program would not run and crashed. I was stuck on the line as marked "// Here is the problem". Plz help me to figure out what's wrong with these codes. thanks!

[jsmith]

Why are you storing pointers to T in your object?

The problem is that elements is an array of pointers.

You have to initialize the pointers before you dereference them

*elements[ i ] = *temp[ i ];

elements[ i ] is not initialized.

[ljin]

Yes, I initialized the elements in the constructor shown as follows:

Set(int initial_size = 4)
:numberOfElements(0),init_size(initial_size){
//default constructor, allocates appropriate heap storage
cout << "Set default constructor\n";
elements = new T*[initial_size];
capacity = initial_size;
index = 0;
}

Yes, I am storing the elements using pointers to original objects.

[jsmith]

Ah, in that case, you want

elements[ i ] = temp[ i ]

(without the dereferences)

or better yet use std::copy

std::copy( temp, temp + capacity, elements );

[ljin]

if using elements[ i ] = temp[ i ], that will cause double deletion/memory leak. That's why we have to write our own copy constructor and assignment operator.

[jsmith]

Are you then wanting a deep copy of the T*?

[ljin]

Deep copay of the T*? I wanna to place many the Person objects into the Set container and do something like this:


Set<Person> s,s2;

s.add(*(new Person("Frank", "[e-mail]", 20, 3, 1967)));
s.add(*(new Student("Eric", "[e-mail]", Date(25, 4, 1958), "Carleton", 12345 )));
s.add(*(new Student("Mary", "[e-mail]", Date(25, 4, 1955), "Carleton", 22234 )));
s.add(*(new Employee("John", "[e-mail]", Date(12, 12, 1970), "Nortel", 99912 )));


s2=s; //Problem is here


the execution will crash when I am doing assignment or copy of the Set class, and the program will stop in the copy constructor or assignment operator of the Person. Actually, the copy constructor and assignment operator of the Person class work fine for me while I am doing assignment or copy of the Person class. There must be something wrong in some where else.

[jsmith]

Ok, step back for a minute.

In your above example code, *(new Foo(...) )

This smacks of a memory leak. Who calls delete on the pointer returned by new? Nobody, because you didn't store the pointer anywhere. So you can never free the memory.

The way your Set<> is implemented, I would expect add to take a T* as parameter, not a T, or const T& (as implied by your above code). At that point, either the Set<> is responsible for deleting the (pointers to) T objects it stores, or the user of Set<> is. Your call. But your answer determines how to write the copy constructor and assignment operator. If Set<> is responsible, then you have to "deep copy" the array. That is, since elements is an array of T*, you actually would need to

for( size_t i = 0; i < capacity; ++i )
    elements[ i ] = new T( *temp[ i ] );

[ljin]

Yes, maybe you get the point. I really did not know what question I should ask so that I just guessed what question it would be from what I've seen. It took me a few days to think about what problem it should be since I was not familiar with "deep copy". After checking what it is, I think that's what I wanna to do. Thanks a lot for your help. I will let you know how it works for me later on.

btw, I did not post all my codes, I do have destructor to delete my Set.

    ~Set(void){ //destructor
        delete [] elements;
    }

[jsmith]

That will only free the memory used by the array of pointers; it will not call the destructor of any T's since it is an array of pointers-to-T, not an array of T's.

[ljin]

I know where I am wrong. The elements[] is an array of pointers to the original objects(T), that does mean that if I delete elements[] and the original objects wont be deleted. My Set<T> container does not own the elements. If my container owns the elements, my destructor will be like this:

   ~Set(void){
      for(int i = 0; i< size; i++)
         delete elements[i];

      delete [] elements;
}

As suggested in the previous post, my assignment operator= should include this line:

elements[i] = temp[i];

and destructor is still like this:

    ~Set(void){ //destructor
        delete [] elements;
    }

That does make sense for me now. Thanks jsmith!