Joe Caraccio Joe Caraccio - 1 month ago 17
C++ Question

C++ linked List assignment Operator

Trying to build an assignment Operator for a single linked list class. I thought I built it correctly, but am still getting a memory leak.

The class consists of a a First and Last variable. And then a Node structure.

The Node structure looks like this:

struct node
{
TYPE value;
node * next;
node * last;
};


My Assignment Operator looks like this, it still has a memory leak

queue& queue::operator=(const queue &rhs){
if(this == &rhs ){

node *ptr = first;
node *temp;
while(ptr != NULL){
temp = ptr;
ptr = ptr->next;
delete temp; // release the memory pointed to by temp
}
delete ptr;


} else{



if (rhs.first != NULL ) {
first = new node(*rhs.first);
first->next = NULL;
last = first;
// first-> value = v.first->value; // copy constructor should have done that

node *curr = first;
node *otherCur = rhs.first;

node *ptr = first;
node *temp;
while(ptr != NULL){
temp = ptr;
ptr = ptr->next;
delete temp; // release the memory pointed to by temp
}


while(otherCur->next != NULL){
curr->next = new node(*otherCur->next);
curr->next->next = NULL;
last = curr->next;
// curr->next->value = otherCur->next->value;
curr = curr->next;
otherCur = otherCur->next;
}
// curr->next = NULL;
}



}
return *this;
}


EDIT:

Copy Constructor(working):

// copy constructor
queue::queue(const queue &v){
if (v.first != NULL ) {
first = new node(*v.first);
first->next = NULL;
last = first;
// first-> value = v.first->value; // copy constructor should have done that

node *curr = first;
node *otherCur = v.first;
while(otherCur->next != NULL){
curr->next = new node(*otherCur->next);
curr->next->next = NULL;
last = curr->next;
// curr->next->value = otherCur->next->value;
curr = curr->next;
otherCur = otherCur->next;
}
// curr->next = NULL;
}


}


Working Destructor:

queue::~queue(){

node *ptr = first;
node *temp;
while(ptr != NULL){
temp = ptr;
ptr = ptr->next;
delete temp; // release the memory pointed to by temp
}


}


Member variables of the .H file:

private:
// fill in here
node * first;
node * last;

Answer

Instead of all of that code, if you have a working copy constructor and destructor, the assignment operator can be implemented easily using copy / swap.

#include <algorithm>
//...
queue& operator=(const queue& v)
{
   queue temp(v);
   std::swap(temp.first, first);
   std::swap(temp.last, last);
   return *this;
}

Basically all that's done is a temporary copy is made through using the copy constructor. Then the member(s) of this are swapped out with the temporary's members. Then at the end, the temporary will be deallocated (the destructor), taking along with it the old data.

I know the code is tiny compared to your attempt, but it solves all the problems pointed out by others in the comments, adds exception safety, etc. and best of all, it works.

But remember, you must have a working, non-buggy copy constructor and destructor (in addition, your copy constructor must not use the assignment operator, which unfortunately many unaware programmers resort in doing). In addition, you must swap all the member variables, so if you add more member variables to your queue class, you need to add a swap for that variable.

See this for information on the copy / swap idiom.