Simple linked list with full set Rule of Five

Question

I'm trying to correctly implement a simple linked list respecting the rule of 5. I get to about 3, although I already have my doubts here, but from there on, I'm on thin ice. As it seems like a fairly common topic, I was surprised I couldn't find a complete example. I've found bits and pieces, but no complete set. So if I get this sorted, it could serve as a future reference as well.

I've added an example class Data for some real life "complexity", because most examples just have a node with a single int and a pointer to the next item.

EDIT: I've completed the class with the code as shown below by PaulMcKenzie and it compiles ok in VS2019, but gives warning on the move constructor and assignment operator: C26439: This kind of function may not throw. Declare it 'noexcept' (f.6).

class Data
{
  public:
    int id;
    string name;
    float[5] datapoints;
};

class Node
{
  public:
    Node(Data d = { 0 }, Node* n = nullptr) : data(d), next(n) {};
    Data& GetData() { return data; }
    Node*& GetNext() { return next; }
  private:
    Data data;
    Node* next;
};

class NodeList
{
public:
    NodeList() :head(nullptr) {}              // constructor
    ~NodeList();                              // 1. destructor
    NodeList(const NodeList& src);            // 2. copy constructor
    NodeList& operator=(const NodeList& src); // 3. copy assignment operator
    NodeList(NodeList&& src);                 // 4. move constructor
    NodeList& operator=(NodeList&& src);      // 5. move assignment operator
    void AddToNodeList(Data data);            // add node
private:
    Node* head;
};

void NodeList::AddToNodeList(Data data)
{
    head = new Node(data, head);
}
NodeList::~NodeList()
{
    Node* n = head, * np;
    while (n != nullptr)
    {
        np = n->GetNext();
        delete n;
        n = np;
    }
}
NodeList::NodeList(const NodeList & src) : head(nullptr)
{
    Node* n = src.head;
    while (n != nullptr)
    {
        AddToNodeList(n->GetData());
        n = n->GetNext();
    }
}
NodeList& NodeList::operator= (const NodeList& src)
{
    if (&src != this)
    {
        NodeList temp(src);
        std::swap(head, temp.head);
    }
    return *this;
}
NodeList::NodeList(NodeList&& src) : head{src.head}
{
    src.head = nullptr;
}
NodeList& NodeList::operator=(NodeList&& src)
{
    if (this != &src)
        std::swap(src.head, head);
    return *this;
}

Answer 1

The first thing to address is that your assignment operator is not correct. You're using the copy / swap idiom, but you forgot to do the copy.

NodeList& NodeList::operator=(NodeList src)  
{
    std::swap(head, src.head);
    return *this;
}

Note the change from a const NodeList& to NodeList src as the argument. This will make the compiler automatically do the copy for us, since the parameter is passed by value.

If you still want to pass by const reference, the following change would need to be made:

NodeList& NodeList::operator=(const NodeList& src) 
{
   if ( &src != this )
   {
       NodeList temp(src);  // copy
       std::swap(head, temp.head);
   }
   return *this;
}

Note the additional test for self-assignment. It really isn't necessary, but may speed up the code (but again, no guarantee).

As to whether this is the most efficient way to do this, that is up for debate -- it all depends on the object. But one thing is for sure -- there will be no bugs, dangling pointers, or memory leaks if you use the copy/swap idiom (correctly).

---

Now onto the move functions:

To implement the missing functions, you should basically remove the contents from the existing object, and steal the contents from the passed-in object:

First, the move contructor:

NodeList::NodeList(Nodelist&& src) : head{src.head} 
{
    src.head = nullptr;
}

All we really want to do is steal the pointer from src, and then set the src.head to nullptr. Note that this will make src destructible, since src.head will be nullptr (and your destructor for NodeList handles the nullptr correctly).

Now for the move-assignment:

Nodelist& operator=(NodeList&& src) 
{
   if ( this != &src )
       std::swap(src.head, head);
   return *this;
}

We check for self assignment, since we don't want to steal from ourselves. In reality, we really didn't steal anything, only swapped things out. However unlike the assignment operator, no copy is done -- just a swap of the internals (this is basically what your incorrect assignment operator that was fixed earlier was doing). This allows the src to destroy the old contents when it's time for the src destructor to be invoked.

Note that after the move (either construction or assignment), the passed-in object is basically in a state that may or may not make the object usable or if not usable, stable (because potentially, the internals of the passed-in object have been changed).

The caller can still use such an object, but with all the risks of using an object that may or may not be in a stable state. Thus the safest thing for the caller is to let the object die off (which is why in the move constructor, we set the pointer to nullptr).