C++ linked List assignment Operator

Question

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;

Please post a [mcve]. The real problem could be anywhere, not just the code you posted. — R Sahu, Oct 10 '16 at 02:48
I know the other parts of my code do not have a memory leak, it is specific to the assignment operator.. tested that already.. — Joe Caraccio, Oct 10 '16 at 02:49
all that code is pretty necessary.. that is just my assignment operator function..Not really sure how I would exclude code.... look I really don't need the policing.. I just need help with this.. :/ — Joe Caraccio, Oct 10 '16 at 02:53
@JoeCaraccio Do you have a working copy constructor? Do you have a working destructor? If the answer is "yes" to both, then the assignment operator is a 4 line function using the `copy / swap` idiom. So if you want an answer, post those functions I mentioned. — PaulMcKenzie, Oct 10 '16 at 02:55
Asking to post a [mcve] serves two purposes: 1. Many programmers are able to locate the problem and solution for it in the process of creating it. 2. You are more likely to get help from an SO user if they can take the code and execute it. — R Sahu, Oct 10 '16 at 02:56
It's obvious that the assignment operator is broken, even without understanding the whole details of the class. Firstly, if `this == &rhs` then, by definition, nothing should happen. Assigning an object to itself is a no-op. Yet, the code goes off and starts deleting stuff. That makes no logical sense. Otherwise, what should happen is: 1. this object's linked list gets deleting. 2. the rhs object's linked list needs to be duplicated and the duplicate copy become this object's linked list. No matter from which angle I look at that code, it doesn't make any sense. — Sam Varshavchik, Oct 10 '16 at 02:59
@PaulMcKenzie Hi Paul, I added them to the original post. Both of the destructor and copy constructor seem to work, as I've tested them many times — Joe Caraccio, Oct 10 '16 at 03:03
@SamVarshavchik I guess I don't understand how to properly structure these then. Iw as under the understanding thats how the assigment constructors were to be built.. — Joe Caraccio, Oct 10 '16 at 03:04
Your copy constructor doesn't set `this->first` if `v.first` is NULL. Also, post the member variables (not functions) that `queue` declares. So far, all I see is `first`, — PaulMcKenzie, Oct 10 '16 at 03:08
@JoeCaraccio: For example, instead of inline code to delete all contents, define a function with a self-documenting name, chosen so that the calling code reads nicely as English. — Cheers and hth. - Alf, Oct 10 '16 at 03:11
@PaulMcKenzie Oh, hmm Okay I'll have to fix that. I also added my member variables to the end of the original post. All it declares is node First and node Last — Joe Caraccio, Oct 10 '16 at 03:13
Write, implement, and test `queue::erase()` that removes all elements from the queue. You should also have an existing `queue::append()` method, or something like that, that appends a new element to the queue. With that in hand, your `operator=`is done! You've written it!!! It's nothing more than `erase()`, followed by iterating over `rhs`'s linked list, and calling `append()` to append a copy of the value to this object's linked list. — Sam Varshavchik, Oct 10 '16 at 03:19

score 1 · Accepted Answer · edited May 23 '17 at 12:33

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& 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 each of those new variables.

See this for information on the copy / swap idiom.

You just made my day.. wow that was a much better solution. Thankyou! — Joe Caraccio, Oct 10 '16 at 03:20

C++ linked List assignment Operator

1 Answers1

Linked