Move Assignment operator c++

Question

I cannot tell what is wrong with my move assignment operator, here is the function. I don't think I am grabbing the data correctly, because when I run the test I get a random negative number and a "you're program has stopped working)

virtual LinkedList<T> &operator=(LinkedList<T> &&other)
{
    cout << " [x] Move *assignment* operator called. " << endl;

    // Delete our own elements
    ListNode<T> *temp1 = _front;
    while (temp1 != nullptr)
    {
        ListNode<T> *n = temp1->getNext();
        delete temp1;           
        temp1 = n;
    }
    // Grab other data for ourselves
    ListNode<T> *temp2 = other._front;
    while (temp2 != nullptr)
    {
        addElement(temp2->getValue());
        temp2 = temp2->getNext();
    }
    // Reset their pointers to nullptr

    other._front = nullptr;
    other._end = nullptr;
    other._size = 0;
    other._last_accessed_index = 0;
    other._last_accessed_node = nullptr;

    return *this;
}

Test Code- this is my teachers test code -

// Use move *assignment* operator
cout << " [x] Test #5: Move *assignment* constructor behavior" << endl;
moved1 = LinkedList<int>{ 6, 7, 8, 9, 10 };
cout << "   [x] Result:" << endl;
cout << "   [x]  Expected:\t6 7 8 9 10" << endl;
cout << "   [x]  Actual:\t\t";
for (int i = 0; i < moved1.getSize(); i++)
{
    cout << moved1.getElementAt(i) << " ";
}
cout << endl << endl;

this is my first time working with move and the move assignment operator. Thanks :)

Answer 1

This is not a proper implementation of a move assignment operator. It looks more like a copy assignment operator (but not a good one, as it leaks memory).

A typical move assignment operator would look more like this instead:

#include <utility>

LinkedList<T>& operator=(LinkedList<T> &&other)
{
    cout << " [x] Move *assignment* operator called. " << endl;

    std::swap(_front, other._front);
    std::swap(_end, other._end);
    std::swap(_size, other._size);
    std::swap(_last_accessed_index, other._last_accessed_index);
    std::swap(_last_accessed_node, other._last_accessed_node);

    return *this;
}

A move assignment operator should not free anything. Move ownership of the source's content to the target object, and vice versa. Let the source object free the target object's previous content when the source object is destroyed after the assignment operator exits, so make sure the class also has a proper destructor implementation:

~LinkedList()
{
    // Delete our elements
    ListNode<T> *temp = _front;
    while (temp != nullptr)
    {
        ListNode<T> *n = temp->getNext();
        delete temp;           
        temp = n;
    }
}

For good measure, here is what the copy constructor, move constructor, and copy assignment operators could look like:

LinkedList() :
    _front(nullptr),
    _end(nullptr),
    _size(0),
    _last_accessed_index(0),
    _last_accessed_node(nullptr)
{
    cout << " [x] Default *constructor* called. " << endl;
}

LinkedList(const LinkedList<T> &src)
    : LinkedList()
{
    cout << " [x] Copy *constructor* called. " << endl;

    ListNode<T> *temp = src._front;
    while (temp != nullptr)
    {
        addElement(temp->getValue());
        temp = temp->getNext();
    }
}

LinkedList(LinkedList<T> &&src)
    : LinkedList()
{
    cout << " [x] Move *constructor* called. " << endl;    
    src.swap(*this);
}

LinkedList(initializer_list<T> src)
    : LinkedList()
{
    cout << " [x] Initialization *constructor* called. " << endl;

    const T *temp = src.begin();
    while (temp != src.end())
    {
        addElement(*temp);
        ++temp;
    }
}

LinkedList<T>& operator=(const LinkedList<T> &other)
{
    cout << " [x] Copy *assignment* operator called. " << endl;

    if (&other != this)
        LinkedList<T>(other).swap(*this);

    return *this;
}

LinkedList<T>& operator=(LinkedList<T> &&other)
{
    cout << " [x] Move *assignment* operator called. " << endl;
    other.swap(*this);        
    return *this;
}

void swap(LinkedList<T> &other)
{
    std::swap(_front, other._front);
    std::swap(_end, other._end);
    std::swap(_size, other._size);
    std::swap(_last_accessed_index, other._last_accessed_index);
    std::swap(_last_accessed_node, other._last_accessed_node);
}

The copy and move assignment operators can actually be merged together into a single implementation, by taking the input object by value and letting the compiler decide whether to use copy or move semantics when initializing that object, based on the context in which the operator is called:

LinkedList<T>& operator=(LinkedList<T> other)
{
    cout << " [x] *assignment* operator called. " << endl;
    swap(other);
    return *this;
}

Answer 2

It's hard to be sure without having the rest of the code, but it looks like you're not correctly clearing the list that is being assigned.

When you do:

// Delete our own elements
ListNode<T> *temp1 = _front;
while (temp1 != nullptr)
{
    ListNode<T> *n = temp1->getNext();
    delete temp1;           
    temp1 = n;
}

You are not actually removing the elements from this. Because of that, moved1 contains nodes that have been deleted, and the execution fails when you start looping on them. What you want to do is remove the nodes from the list just before deleting them.

The way to go would be:

// Remove our own elements
ListNode<T> *temp1 = _front;
while (temp1 != nullptr)
{
    ListNode<T> *n = temp1->getNext();
    pop();
    delete temp1;           
    temp1 = n;
}

And have a method like:

void pop() // removes the first element from the list
{ 
    _front = _end._front; 
    _end = _end._end; 
    --_size; 
}

Of course the definition of pop will depend on your full implementation of the class. If you are storing pointer to the object that were given to you, you should probably not delete those. However if you are using an additional wrapper such a ListNode, your pop method should delete them - although in the case of a wrapper, it's even better not to use pointers at all.

You can have a look at std::list::pop_front for more information.