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 :)

What does the debugger show you when you step through the code? — Ken White
– Ken White, Commented Sep 14, 2017 at 1:09
it will run through everything, but when I run my test code it breaks when trying to receive the data in the list @KenWhite — sarah campolt
– sarah campolt, Commented Sep 14, 2017 at 1:11
Can you post the test code - or preferably a minimum subset of it that demonstrates the problem? — norlesh
– norlesh, Commented Sep 14, 2017 at 1:12
If you step through the code, watching the variables, you'll see the issue. Either you're not stepping through the code, or you're not doing it correctly. Step through, line by line, and watch the values and contents of the variables. And if you have test code, you should include a minimal reproducible example here in your post so we have it as well. — Ken White
– Ken White, Commented Sep 14, 2017 at 1:14
Oh, and the bug looks rather obvious. After removing this's data, the front_ pointer is not reset, and it's left pointing to deleted objects. Hello memory corruption! — Sam Varshavchik
– Sam Varshavchik, Commented Sep 14, 2017 at 1:20

Remy Lebeau · Accepted Answer · 2017-09-14 02:38:08Z

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;
}

edited Sep 14, 2017 at 2:38

answered Sep 14, 2017 at 1:45

Remy Lebeau

609k36 gold badges516 silver badges875 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

sarah campolt Over a year ago

Thanks so much for everything you have added! For this exercise, I think the "move assignment operator" is just supposed to simulate the what the move assignment operator would do using "copy" methods, if that made any sense. At the moment, my code is outputing 1 2 3 4 5 when I need 10 9 8 7 6, if you could offer any clarification for this, it would be greatly appreciated.

sarah campolt Over a year ago

Again, thanks for all this information though, I was having trouble understanding the concepts of everything :)

Remy Lebeau Over a year ago

Implementing a move assignment operator using copy semantics is wrong. That defeats all of the benefits of move semantics. If you are going to do that, you may as will just implement a copy assignment operator by itself. The code I have given you should not be outputting 1 2 3 4 5 if it was given 6 7 8 9 10. If you are getting 1 2 3 4 5 after move assignment, you are doing something wrong.

Flynsee · Accepted Answer · 2017-09-14 01:42:54Z

0

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.

edited Sep 14, 2017 at 1:42

answered Sep 14, 2017 at 1:35

Flynsee

5964 silver badges17 bronze badges

3 Comments

sarah campolt Over a year ago

Thanks for your response! When I run the code as you suggested, the list has the numbers 1 2 3 4 5 6 7 8 9 10, and I want the output of 10 9 8 7 6, but right now my output is 1 2 3 4 5

Flynsee Over a year ago

Is it because removeElementAt already deletes it? In which case you don't need to delete it again in the move assignment operator.

sarah campolt Over a year ago

Thats exactly it, I realized my mistake and updated the response! :)

Collectives™ on Stack Overflow

Move Assignment operator c++

2 Answers 2

3 Comments

3 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related