Assignment Operator for Doubly Linked List C++

Question

I have been having trouble getting my assignment operator for a doubly-linked list to work properly. The operator works correctly when the rhs is an empty list, but if it is populated, it does not work and throws an exception error saying "read access violation".

Here is my main routine that will not run.

#include <cstdlib>

#include "linkedlist.h"

using namespace std;

int main()
{
    LinkedList e1;
    e1.insertToFront("A");
    e1.insertToFront("B");
    e1.insertToFront("C");

    LinkedList e2;

    e2.insertToFront("Please work");

    e1 = e2; //Expecting e1 to now just hold "Please work".

    system("pause");
}

Here is the assignment operator itself (in a separate header file).

// assignment operator
const LinkedList& LinkedList::operator=(const LinkedList& rhs)
{
    Node* temp;
    temp = head;
    Node* forward;
    forward = new Node;

    while (temp != nullptr) // clear memory of target variable
    {
        forward = temp->next;
        delete temp;
        temp = forward;
    }

    if (rhs.head == nullptr)
    {
    head = nullptr;
    tail = nullptr;
    } 
      //GOOD THROUGH THIS PNT.

    else
    {       
        temp = rhs.head;

        while (temp != nullptr)
        {
            this->addToEnd(temp->value);
            temp = temp->next;
        }
    }

    return *this;
}

And here is the addToEnd function that I call as well as the Node struct.

void LinkedList::addToEnd(const ItemType& val) 
{
    Node* temp;
    temp = new Node;
    temp->value = val;

    if (this->head == nullptr)
    {
        head = temp; // make new node head if list is empty
        head->next = nullptr;
        head->prev = nullptr;
        tail = temp;
    }

    else
    {
        temp->prev = tail; // otherwise point current tail towards temp
        temp->next = nullptr;
        tail->next = temp;
        tail = temp;
    }

    return;
}

////////////////////////////////////////////////////////////////////////

struct Node
{
    ItemType value;
    Node* next;
    Node* prev;
};

This question may be liable to downvotes since you provided too much code. A [mcve] is recommended. A "read access violation" could be a segmentation fault — Ṃųỻịgǻňạcểơửṩ, Jul 12 '18 at 21:04
Side note: you `new` a Node to assign to forward, but you never delete it. This will be a memory leak. — Ben Jones, Jul 12 '18 at 21:05
For an often-useful alternative view of writing an assignment operator give [What is the copy-and-swap idiom?](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) a read. — user4581301, Jul 12 '18 at 21:23

score 3 · Answer 1 · answered Jul 12 '18 at 21:17

3

You delete the old nodes, but you neglect to set head and tail to nullptr, so those pointers still point to deleted objects. Then you try to add elements to that deleted list, and get Undefined Behavior.

answered Jul 12 '18 at 21:17

Beta

96,650
16
149
150

Remy Lebeau · Answer 2 · 2018-07-12T21:59:47.653

When you are clearing out the existing nodes, you are not resetting your head and tail pointers to nullptr before you start copying values from the source list. So you are adding new Nodes to your list using invalid pointers.

You also have a small memory leak, as you are allocating a new Node for the forward variable and then immediately reassign forward to point at another Node if the source list is not empty. You never delete the Node you allocate with new. You should not be allocating ANYTHING when clearing out the existing nodes.

To make things a little safer, you should wrap the clearing of the list in a separate method of its own, and then you can call that method whenever needed (don't forget in the destructor, not just in the assignment operator=).

It would be even better if you implement your assignment operator= in terms of your copy constructor (you do have one, right?) using the copy-and-swap idiom. And since you are clearly using C++11 or later, you should also implement a move constructor as well. These steps will greatly simplify your operator= implementation and make it safer to use.

Try this:

class LinkedList
{
public:
    LinkedList() = default;
    LinkedList(const LinkedList& src);
    LinkedList(LinkedList&& src);
    ~LinkedList();
    ...
    LinkedList& operator=(LinkedList rhs);
    ...
private:
    Node *head = nullptr;
    Node *tail = nullptr;
    ...
};

#include <utility>

LinkedList::LinkedList(const LinkedList& src)
{
    Node* temp = src.head;
    while (temp)
    {
        addToEnd(temp->value);
        temp = temp->next;
    }
}

LinkedList::LinkedList(LinkedList&& src)
    : head(src.head), tail(src.tail)
{
    src.head = src.tail = nullptr;
}

LinkedList::~LinkedList()
{
    clear();
}

void LinkedList::clear()
{
    Node *temp = head;
    head = tail = nullptr;
    while (temp)
    {
        Node *forward = temp->next;
        delete temp;
        temp = forward;
    }
}

LinkedList& LinkedList::operator=(LinkedList rhs)
{
    std::swap(head, rhs.head);
    std::swap(tail, rhs.tail);
    return *this;
}

You can also simplify your insertToFront() and addToEnd() methods a little bit, too:

struct Node
{
    ItemType value;
    Node* next = nullptr;
    Node* prev = nullptr;

    Node(const ItemType& val) : value(val) {}
};

void LinkedList::insertToFront(const ItemType& val) 
{
    Node* temp = new Node(val);

    if (!tail)
        tail = temp; // make new node tail if list is empty

    if (head)
    {
        temp->next = head; // point current head towards temp
        head->prev = temp;
    }

    head = temp;
}

void LinkedList::addToEnd(const ItemType& val) 
{
    Node* temp = new Node(val);

    if (!head)
        head = temp; // make new node head if list is empty

    if (tail)
    {
        temp->prev = tail; // point current tail towards temp
        tail->next = temp;
    }

    tail = temp;
}

Assignment Operator for Doubly Linked List C++

2 Answers2