Stack implementation with move semantics

I have implemented a basic stack just for fun. I've just started learning about move constructors and assignment and move semantics as a whole, so I would really appreciate some feedback as to whether I've done it right, and if not, some tips as to how I should do it would be most welcome!

I'm also following the example set by the STL, so I'm trying to build the class in an STL-like manner.

The stack class is in a different namespace, which I've removed from this example, so conflicts with std::stack will not appear.

The _stack_node is also in a different details namespace, so it is not exposed to the user.

```
#include
#include
#include
#include
#include
#include

//FORWARD DECLARATION
template
class stack;

template
class _stack_node
{
private:
using node = _stack_node;

friend class stack;

private:
T _key;
node *_next;

public:
//COPY CONTRUCTORS
constexpr _stack_node()
: _key(0), _next(nullptr)
{}

constexpr _stack_node(const T &new_key)
: _key(new_key), _next(nullptr)
{}

constexpr _stack_node(const T &new_key, node *new_node)
: _key(new_key), _next(new_node)
{}

//MOVE CONSTRUCTORs
constexpr _stack_node(T &&new_key)
: _key(std::forward(new_key)), _next(nullptr)
{}

constexpr _stack_node(T &&new_key, node *new_node)
: _key(std::forward(new_key)), _next(new_node)
{}

//DESTRUCTOR
~_stack_node() {}

//COPY ASSIGNMENT
node &operator=(const node &rhs)
{
this->_key = rhs._key;
this->_next = rhs._next;
return *this;
}

//MOVE ASSIGNMENT
node &operator=(node &&rhs)
{
this->_key = std::move(rhs._key);
this->_next = std::move(rhs._next);
return *this;
}
};

template
class stack
{
private:
using node = _stack_node;

public:
using value_type = T;
using size_type = std::size_t;
using reference = T&;
using const_reference

General Comments

Don't like the naming of your class _stack_node or stack.

The first one has a leading underscore. This is usually a bad sign. Do you know all the rules for leading underscore? More importantly, does everybody that reads your code understand all the rules?

Also it is traditional to use a leading uppercase letter for user defined types.

Also the same applies to all your method and member variable names. The leading underscore is distracting. Also this says to me that you needed to distinguish local variables from member variables which indicates that you are having trouble writing clear readable code (and need an artificial way to help you discriminate).

Its very C like to put the next to the name. Over the years it has become more traditional in C++ code to put the next to the type. This is because it is type information and type information is the most important part of C++.

node *_top;

Note: The same rule applies to & and &&. They belong with the type.

Your use of this is an indication that you are having trouble naming your members well enough to distinguish them from local variables. It is uncommon in C++ code to see this. If you have your compiler warning set correctly then shadowed variables will generate a warning and warnings should be treated as errors (as they are logical errors in your thinking).

if (this->_top == nullptr)

_stack_node

Is there ever going to be a case where you have an empty node?

//COPY CONTRUCTORS
    constexpr _stack_node()
        : _key(0), _next(nullptr)
    {}

If so then maybe you need a special node to handle this because not all types are going to be construct-able with int(0). I did not see anywhere in your code where you used this constructor so may as well delete it.

I hate the attempt at saving vertical space:

constexpr _stack_node(const T &new_key)
        : _key(new_key), _next(nullptr)
    {}

I don't think it hinders readability (and it definitely does not help the compiler). So why make the code harder to read. Like the one variable declaration per line try and use one member per line initialization.

This constructor and the last one can be combined into a single constructor using default values (just make the new_node default to nullptr).

constexpr _stack_node(const T &new_key, node *new_node)
        : _key(new_key), _next(new_node)
    {}

Same here:

constexpr _stack_node(T &&new_key, node *new_node)
        : _key(std::forward(new_key)), _next(new_node) 
    {}

If your code is not different from the compiler generated code then there is no point in adding cognitive burden to the reader of your class.

//DESTRUCTOR
    ~_stack_node() {}

May as well delete this destructor it has the same result as the compiler generated version.

Move Semantics (Will go into more details about the move semantics below with the stack class).

stack

Not sure why you throw and catch in the same function!

void _throw_stack_empty() const
    { //in case we try to pop an empty stack!!
        try
        {
            if (this->_top == nullptr)
                throw std::out_of_range("You tried to pop an empty stack!"
                                        " Why the hell would you do that?");
        }
        catch (const std::out_of_range &rang)
        {
            std::cerr << "blew::stack: " << rang.what();
            std::abort();
        }
    }

Just test and throw.

If the exception is not caught then the application will exit. If the exception is caught by the application then it is non of your business (it definitely should not be the business of a container class to arbitrarily decide that the application should exit (unless you can show corruption).

You should finish the state update of your object before calling delete.

void _delete_node()
    {
        this->_throw_stack_empty(); //check and throw if stack is empty

        node *temp_top = this->_top; 
        this->_top = this->_top->_next;

        // Though it is rare for delete to throw it can happen.
        // If it does then you will not decrement the size of your
        // class and thus your object is left in an inconsistent state.
        delete temp_top;

        --this->_size; //decrease size
    }

When modifying an object it should be done in three phases.

1) Create new state into temporary object.s
       Thus if the creating the state is dangerous and causes an exception
       the state of your object will be unchanged.

    2) Modify the state of your object in an exception safe manner.

    3) Clean up.
       Only destroy resources and other objects once the state of your
       object is a completely consistent state. This way if there is an
       exception your object is going to maintain the strong exception
       guarantee.

You suffer from the same problem in _clear_all_nodes() as _delete_node()

```
void _clear_all_nodes()
{
while

StackExchange Code Review Q#111624, answer score: 12