Doubly linked std::list simple implementation

I've implemented a simple doubly linked list structure. I would appreciate all criticism relevant to code, style, flow, and so forth.

How to implement end iterator and pop_back/pop_front properly? Where memory may leak?

P.S.I didn't make it template, because this is simple version version.

```
#include
#include

using T = int;

class List {
/// Nodes
public:
class Node {
public:
T data;
Node *next;
Node *prev;

public:
explicit Node(Node next_, Node prev_) : next(next_), prev(prev_) {}

explicit Node(const T &_data, Node next_, Node prev_) :
data(_data), next(next_), prev(prev_) {}

~Node() {}
};
///

/// Iterators
public:
class Iterator : public std::iterator {
public:
Node *position;
Node *tail;

public:
Iterator(Node *node) : position(node) {}

Iterator(Node node, Node _tail) : position(node), tail(_tail) {
}

Iterator &operator++() noexcept {
position = position->next;
return *this;
}

Iterator operator++(int) noexcept {
auto old = *this;
position = position->next;
return old;
}

Iterator &operator--() noexcept {
if (position == nullptr) position = tail;
else
position = position->prev;
return *this;
}

Iterator operator--(int) noexcept {
auto old = *this;
if (position == nullptr) position = tail;
else
position = position->prev;
return old;
}

T &operator*() const noexcept {
return (position->data);
}

bool operator!=(Iterator other) noexcept {
return position != other.position;
}

bool operator==(Iterator other) noexcept {
return position == other.position;
}

~Iterator() {}
};
///

/// list

Take care to be explicit about compiler generated functions

Effectively your List class violates the Rule of Three/Five/Zero, thus the compiler generated copy/move constructors will create unexpected side effects by implementing a shallow copy mechanism.

Let's suppose you have code like

int main() {
    List l1;
    List l2;

    l1.push_back(5);
    l1.push_back(42);
    l1.push_back(21);

    l2 = l1; // Copy the list
    *l1.begin() = 80; // Change the original list

    for(auto value : l2) {
        std::cout << value << std::endl;
    }
}

The output is

80
42
21

I doubt you really intended that. See the Live Demo please.

Why not making your class a template?

using T = int;

looks strange.

Why not making the List class a template like

template
class List {
    // ...
};

Compiles and works the same way here

Prefer smart pointers instead of raw pointers

You should prefer using smart pointers instead of managing new and delete yourself.

I have evolved the demo from above with changing the relevant parts

class Node;
class Node {
public:
    T data;
    std::shared_ptr next;
    std::shared_ptr prev;

public:
    explicit Node(std::shared_ptr next_, std::shared_ptr prev_) : next(next_), prev(prev_) {}

    explicit Node(const T &_data, std::shared_ptr next_, std::shared_ptr prev_) :
            data(_data), next(next_), prev(prev_) {}

    ~Node() {}
};

class Iterator : public std::iterator {
public:
    std::weak_ptr position;
    std::weak_ptr tail;

public:
    Iterator(std::shared_ptr node) : position(node) {}

    Iterator(std::shared_ptrnode, std::shared_ptr_tail) : position(node), tail(_tail) {
    }

    Iterator &operator++() noexcept {
        position = position.lock()->next;
        return *this;
    }

    Iterator operator++(int) noexcept {
        auto old = *this;
        position = position->next;
        return old;
    }

    Iterator &operator--() noexcept {
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return *this;
    }

    Iterator operator--(int) noexcept {
        auto old = *this;
        if (position == nullptr) position = tail;
        else
            position = position->prev;
        return old;
    }

    T &operator*() const noexcept {
        return (position.lock()->data);
    }

    bool operator!=(Iterator other) noexcept {
        return position.lock().get() != other.position.lock().get();
    }

    bool operator==(Iterator other) noexcept {
        return position.lock().get() == other.position.lock().get();
    }

    ~Iterator() {}
};

Solve the shallow copy problem

As mentioned in my 1st point you have a problem with The Rule of Three/Five. Your List class will be shallow copied. So you should provide the relevant operations.

This will be a lot easier using smart pointers as mentioned in my previous point

List(const List& rhs) {
    copy(rhs);
}

List(List&& rhs) {
    move(std::move(rhs));
}

List& operator=(const List& rhs) {
    copy(rhs);
    return *this;
}

List& operator=(List&& rhs) {
    move(std::move(rhs));
    return *this;
}

void copy(const List& rhs) {
    clear();
    for(auto value : rhs) {
        push_back(value);
    }
}

void move(List&& rhs) {
    head = std::move(rhs.head);
    tail = std::move(rhs.tail);
}

void clear() {
    std::shared_ptr temp = head;
    std::shared_ptr next;
    do {
        if(temp) {
            next = temp->next;
        }
        temp = nullptr;
    } while(next);
}

~List() {
}

Output is as expected for the example mentioned in the 1st paragraph:

5
42
21

Again you can check the improved code here.

StackExchange Code Review Q#158129, answer score: 3