Linked list operator= When Move Assigning Then All Elements Are Moved

Question

I'm trying to write Lined list in C++, but some tests fails.

One of those says:

GivenNonEmptyCollection_WhenMoveAssigning_ThenAllElementsAreMoved

And second:

GivenNonEmptyCollection_WhenMovingToOther_ThenAllItemsAreMoved

Here's how I implement operator=

LinkedList& operator=(const LinkedList& other)
{
       if(this!=&other)
       {

        while (!isEmpty()) 
          erase(begin());
        for (auto it = other.begin(); it != other.end(); it++)
          append(*it);
      }
return *this;}

And second one:

 LinkedList& operator=(LinkedList&& other)
  {
/* SELF ASSIGNMENT CHECK */
    if(this!=&other)
    {
        while (!isEmpty()) 
        erase(begin());
        while (!other.isEmpty()) 
        {
            append(*(other.begin()));
             other.erase(other.begin());
        }
    }
    return *this;
  }

Here's something about class Linked list and struct Node:

template <typename Type>
class LinkedList
{
    struct Node
    {
        Node* prev;
        Node* next;
        Type* data;
        Node()
        {
            data = nullptr;
            prev = nullptr;
            next = nullptr;
        }
        Node(const Type val)
        {
            data = new Type(val);
            prev = nullptr;
            next = nullptr;
        }
        ~Node()
        {
            prev = nullptr;
            next = nullptr;
            delete data;
        }
    };


private:
    Node *head;
    Node *tail;
    size_type length;

public:

LinkedList(): head(nullptr), tail(nullptr), length(0)
  {
      head = new Node;
      tail = new Node;
      head->next = tail;
      tail->prev = head;
  }


(...)

I have no idea what's wrong with that.

*You have no idea what's wrong with that?* than how can you make sure it is wrong? — apple apple, Nov 09 '17 at 15:09
Probably unrelated: When moving elements you are not supposed to copy and destroy every element. You are supposed to so something like `std::swap(this->head, other.head);` and same with `tail` and `length`. That way you can move lists in constant time instead of linear time. — nwp, Nov 09 '17 at 15:11
@appleapple Because the tests fail. That's what tests are for. — nwp, Nov 09 '17 at 15:11
@nwp well, I mean, OP should at least tell us what's wrong. (I don't see a test in question). for example, if the test is `Test(){Fail();}` than all the code can be OK. — apple apple, Nov 09 '17 at 15:13

molbdnilo · Answer 1 · 2017-11-09T15:33:54.743

1

You're copying and deleting the original list, but you should move it.
In this case, this means "stealing" the data from the other list.

It should look more like this:

LinkedList& operator=(LinkedList&& other)
{
    if(this!=&other)
    {
        // Assuming the existence of 'LinkedList::clear', which empties the list.
        // Replace with the name you chose for that function.
        clear();  
        head = other.head;
        other.head = nullptr;
        tail = other.tail;
        other.tail = nullptr;
        length = other.length;
        other.length = 0;
    }
    return *this;
  }

and your move constructor should be changed similarly.

edited Nov 09 '17 at 15:33

answered Nov 09 '17 at 15:18

molbdnilo

64,751
3
43
82

@ChrisDrew You are probably right. I missed that. I would add a comment to indicate that. – nwp Nov 09 '17 at 15:28
surely just a memberwise swap is all that is needed? delegating the cleanup to `~other` seems better – Caleth Nov 09 '17 at 15:35
1

@Caleth I personally expect to be left with nothing when I move out of an object, not with the object that was moved into. YMMV, and all that. – molbdnilo Nov 09 '17 at 15:36
And that forces you to do the self alias check. If you `swap(*this, other)` then self move-assignment is fine – Caleth Nov 09 '17 at 15:39
@Caleth There are various pros and cons of using `swap` for move assignment. See for example [here](https://stackoverflow.com/questions/6687388/why-do-some-people-use-swap-for-move-assignments) and [here](http://scottmeyers.blogspot.co.uk/2014/06/the-drawbacks-of-implementing-move.html). – Chris Drew Nov 09 '17 at 16:06

Linked list operator= When Move Assigning Then All Elements Are Moved

1 Answers1