Beginner c++ sorted link list using =operator for deep copy

Question

I am trying to use the overload operator constructor, but having some trouble with exceptions. Need a little help.

I have an existing deep copy that works fine, but when trying to use it from a copy using the overload =operator, it is throwing all sorts of errors.

    ListClass& ListClass::operator=(const ListClass& rhs)
    {
    if (this != &rhs)
    {
        while(this->head != NULL)
        {
            remove (1);
        }
        this->head = new ListNode;
        assert(head != NULL);  // check allocation

        this->head->item = rhs.head->item;

        // copy rest of list
        ListNode *newPtr = head;  // new list pointer

        ListNode ;
                                  // newPtr points to last node in new list
                                  // origPtr points to nodes in original list
        for (ListNode *origPtr = rhs.head->next;
            origPtr != NULL;
            origPtr = origPtr->next)
        {
            newPtr->next = new ListNode;   // link new node to end of list
            assert(newPtr->next != NULL);
            newPtr = newPtr->next;

            newPtr->item = origPtr->item;  // copy the data
            newPtr->next = NULL;
        }
        this->head = rhs.head;
    }
    return *this;
  }

Do you have a working copy constructor and destructor for your class? If so, then the assignment operator need not be written in this way. Just using a few `swap`'s is all you need. — PaulMcKenzie, Mar 10 '17 at 23:44
I would suggest you implement this function first (copy constructor): `ListClass(const ListClass& rhs);`. Then implement this function next (destructor): `~ListClass();`. Once you have those two functions, the assignment operator becomes 3 lines of code if `head` is the only member of `ListClass`. — PaulMcKenzie, Mar 11 '17 at 00:02

score 0 · Answer 1 · answered Mar 10 '17 at 23:56

0

Firstly delete this->head = rhs.head; You're copying all elements of rhs to this but at the end you assign old head to the head of new object and return it. Now you have new ListClass object that points to the old elements.

answered Mar 10 '17 at 23:56

K. Macieja

23
1
5

Thank you for the feedback. I deleted that line, but now it's returning a debug error "rhs.head was nullptr" – user3605629 Mar 11 '17 at 01:12

score 0 · Answer 2 · answered Mar 11 '17 at 03:10

0

This is working below: comments would be great!

SortListClass& SortListClass::operator=(const SortListClass& rhs)
{

// TODO
// Similar to Copy Constructor, except
// - Avoid self-assignments such as  “X = X;”
// - Delete existing this-instance content before 
//   making this-instance a copy of the rhs instance
if (this != &rhs)
{
while (!isEmpty())
{
    remove(1);
}
size = rhs.size;

}
if (rhs.head == NULL)
        head = NULL;  // ORIGINAL LIST IS EMPTY
else
{
    // copy first node
    this->head = new SortListNode;
    assert(head != NULL);  // check allocation

    this->head->item = rhs.head->item;

    // copy rest of list
    SortListNode *newPtr = head;  // new list pointer


                                  // newPtr points to last node in new list
                                  // origPtr points to nodes in original list
    for (SortListNode *origPtr = rhs.head->next;
        origPtr != NULL;
        origPtr = origPtr->next)
    {
        newPtr->next = new SortListNode;   // link new node to end of list
        assert(newPtr->next != NULL);
        newPtr = newPtr->next;

        newPtr->item = origPtr->item;  // copy the data
        newPtr->next = NULL;
    }

}
return *this;


 };

answered Mar 11 '17 at 03:10

user3605629

11
5

Look at your comments in the answer you posted: *// TODO // Similar to Copy Constructor, except // - Avoid self-assignments such as “X = X;” // - Delete existing this-instance content before // making this-instance a copy of the rhs instance* -- To avoid all of this, see my answer. – PaulMcKenzie Mar 11 '17 at 06:19
Thank you. That does work perfectly, but for the purpose of this assignment, I believe my teacher wanted us to use the this assignment method. – user3605629 Mar 11 '17 at 07:01
My answer was provided so that other people who don't have ridiculous restrictions put on them can see how simple it is to implement a copy assignment operator. If your copy constructor is correct, I can guarantee that my answer will work. Your answer will **not** work if `new` throws an exception. You've messed up your object by removing all the elements before `new` is called, and there is no way to rollback all the changes you did to your class. So you still have an issue. – PaulMcKenzie Mar 11 '17 at 14:02
So basically, the way to write the copy assignment operator safely is to first, create a totally independent, brand new, copy of `*this`, then remove the old data from `*this`, and then assign the new copy to `*this` using simple pointer assignment (similar to the answer I posted with the difference being that the old data is removed last). Removing elements before calling `new` leads to an "oops" moment if `new` goes awry. – PaulMcKenzie Mar 11 '17 at 14:12
*I believe my teacher wanted us to use the this assignment method.* -- This assignment method your teacher wants you to use is broken. It would be a disservice to you if you were told this was correct. – PaulMcKenzie Mar 11 '17 at 14:22

score 0 · Answer 3 · edited May 23 '17 at 11:53

@user3605629, Looking at your own answer, the simplest way to implement your assignment operator, given you already have a copy constructor is as follows:

#include <algorithm>
//...
SortListClass& SortListClass::operator=(const SortListClass& rhs)
{
   SortListClass temp(rhs); // create a temporary donor object from rhs
   std::swap(temp.head, head);  // swap out our current data with donor data
   std::swap(temp.size, size);  // swap out our current size with donor size
   return *this;
}  // the donor object dies off with our old data

Assuming that the only member variables in your SortListClass are head and size, and you've implemented a working copy constructor and destructor, this is all you need to do to avoid all of those issues mentioned in your comments (listed here):

// Similar to Copy Constructor, except
// - Avoid self-assignments such as  “X = X;”
// - Delete existing this-instance content before 
//   making this-instance a copy of the rhs instance

This uses the copy / swap idiom to implement the assignment operator. If you have other member variables other than head and size, then you just swap those also. Make sure that you swap all the members -- any left out will cause issues with incomplete copies being created.

This is by far the easiest, safest, and practically foolproof way of implementing the copy/assignment operator. The deep copy is done on the very first line by creating a temporary object using the copy constructor. So again, this requires a working copy constructor that does not in any way call the assignment operator.

The old data is removed by having the SortListClass destructor called on the temporary object when it goes out of scope at the end of the function, since the temporary object now has the old data due to the swap's being done between the current object and temp.

Edit: Another thing this answer guarantees is that it is exception-safe. Note that your answer changes your SortListClass object before new is called. If new throws an exception in your answer, the object has been corrupted and there is no way to bring back the data you removed previously.

The answer above makes sure this doesn't happen by creating a temporary object using the copy constructor. If the temporary object fails to be created due to new failing in the copy constructor, the original object is not altered in any way.

Beginner c++ sorted link list using =operator for deep copy

3 Answers3