My first implementation of a linked list in C++

This is my very first implementation of a full-fledged ADT, which could potentially be use-ready. Now, I'm still learning, therefore I would like to ask you what I can do to further improve the following code.

```
#ifndef LIST_H
#define LIST_H

#include
#include
#include
#include

template
class List {
class Node {
friend List;

Node *m_next;
typename std::aligned_storage::type m_data;

Node() : m_next(nullptr) {}
Node(const T &data, Node *next) : m_next(next) {
new(&m_data) T(data);
}
};

Node *m_sentinel,
*m_head,
*m_tail;

size_t m_length;

template
class Iterator_ {
friend List;

using NodeType = typename std::conditional::type;
using ValueReferenceType = typename std::conditional::type;
NodeType m_node;

Iterator_(NodeType node) : m_node(node) {}

public:
Iterator_(const List &list, size_t index = 0) : m_node(list.m_head) {
if (index >= list.m_length) throw std::out_of_range("Iteration out of bounds");
operator+(index);
}

Iterator_ &operator++() {
assert(m_node != m_node->m_next);
m_node = m_node->m_next;
return *this;
}

Iterator_ operator++(int) {
Iterator old = *this;
operator++();
return old;
}

Iterator_ &operator+(size_t index) {
for (size_t cur_index = 0; cur_index m_next); // Dereferencing a sentinel (end iterator)
return reinterpret_cast(m_node->m_data);
}
};

using Iterator = Iterator_;
using ConstIterator = Iterator_;

public:
List() : m_sentinel(new Node()), m_head(m_sentinel), m_tail(m_sentinel), m_length(0) {}

List(const List ©) : List() {
append(copy);
}

List(List &&

Resource Management.

Copy the list. You have obeyed the rule of three. But you don't provide the strong exception guarantee on your copy assignment operator. Basically this means that assignment either works and you make a copy of the new list from the source into the dest, BUT if it fails then the object i sleft unchanged.

List &operator=(const List ©) {

        clear();         //  Here you destroy the current state.
        append(copy);    //  If this fails.
                         //  you can not reset the state of the object.
        return *this;
    }

This is called the basic exception guarantee. That's fine but you need to document it because most people expect the strong exception gurantee. Also using the copy and swap idiom mkae providing this gurantee easy,

// Copy assignment with copy and swap.
    List& operator=(List const& rhs)
    {
        List  tmp(rhs);    // Copy:  Make  copy into a temporary.
        rhs.swap(*this);   // Swap;  (exception safe)
        return *this;
    }

Moving the list. You have the move constructor. But I am not convinced it is correct. But you also missed the move assignment operator.

List(List &&move) : List() {
        std::swap(m_sentinel, move.m_sentinel);
        std::swap(m_head,     move.m_head);

        // Why not swap these members?
        m_tail   =            move.m_tail;
        m_length =            move.m_length;
    }

The requirement is that the object that was moved is left in a valid state (Though indeterminate). I am not convinced this is valid state. If I call methods on this object it should not cause bad things to happen (though I should not expect to know what could happen).

Currently:

List  list;
 list.append(6);

 // Some badly written function.
 // That **may** take ownership of the list but it may not.
 sendListSomewhere(std::move(list));

 // If the list was moved.
 // This will append `5` to the list that was moved
 // not to the current list.
 //
 // If the list was not moved
 // then it adds 5 to the current list locally.
 if (list.empty()) {
    list.append(5);
 }

Also the move assignment operator has not been written.

List  z;
 z = std::move(list);

Also you should mark the move operator as non throwing (by using noexcept). This will allow the standard containers to use your list in the most optimal way possible and still provide the strong exception guarantee.

std::vector>   x;
x.puhs_back();
x.push_back();  // If List does not have an noexcept move constructor
                // then vector must fall back to using copy constructor
                // when the internal structure of x is resized.
                //
                // Alternatively if the move constructor is noexcept
                // then a resize can use the move constructor
                // and still provide the strong exception guarantee.

Sentinel

Node *m_sentinel,
     *m_head,
     *m_tail;

You don't really need head/tail if you are using a sentinel. The head is the element after the sentinel. The tail is the one before the sentinel. There is no need to keep track of these values. (Now I see you are using a singly linked list so no previous. So you will need tail (just not head).

Also don't be lazy with the declarations.

Node* m_sentinel,
Node* m_head,
Node* m_tail;

In C++ (unlike C) the * or the & tend to be placed with the type not the variable (its part of the type and the type is much more important in C++ than C).
Placement New is not needed.

typename std::aligned_storage::type m_data;
 Node(const T &data, Node *next)
     : m_next(next)
 {
     new(&m_data) T(data);
 }

I would just have written:

T data;
 Node(Node* next, T const& data, Node* next)
      : m_next(next)
      , m_data(data)
 {}

I would also add a move version of the Node constructor.

Node(Node* next, T&& data)
      : m_next(next)
      , m_data(std::forward(data)
 {}

Also for complex types of T you should allow the object T to be built in place by forwarding the constructor arguments for T.

template
 Node(Node* next, Args...&& args)
      : m_next(next)
      , m_data(std::forward(args)...)
 {}

Hate the use of assert.

It means that production code runs and behaves differently from test code. In my opinion code should behave the same way no matter what (otherwise things become hard to diagnose when things go wrong).

Iterator_ &operator++() {
                assert(m_node != m_node->m_next);
                m_node = m_node->m_next;
                return *this;
            }

Not sure why you are using reinterpret_cast.

ValueReferenceType &operator*() const {
                return reinterpret_cast(m_node->m_data);
            }

There should be no need to use this type.

Another insert.

```
List &insert(const T &element, size_t index = 0) {
if (index > m_length) {
assert(0);
index = m_length;

StackExchange Code Review Q#125955, answer score: 9