Superior improved stack implementation using a linked list

Question

This is a followup to

• Improved stack implementation using a linked list
• Simple stack implementation using linked list

Please review my hopefully improved stack implementation

• As suggested by some answers to my previous code, some of the functions can be overloaded to provide versions of return by value, return by reference and const or pass by value, pass by reference and const, but the function definitions are almost the same so I did not add it here.
• Regarding pointers, I know std::unique_ptr will be better and I already saw an implementation through an answer to my previous code, but I like to play with normal pointers right now.

template<class T>
class Stack {
    using stacksize = std::size_t;
public:
    Stack() : first{nullptr}, n{0} {}
    stacksize size() const { return n; }
    bool empty() const { return n == 0; }
    Stack(const Stack&);
    Stack(Stack&&);
    Stack& operator=(Stack);
    Stack& operator=(Stack&&);
    T& operator[](const stacksize& i) {
        Node* traverse = first;
        stacksize x = 0;
        while (x < i) {
            traverse = traverse->next;
            ++x;
        }
        return traverse->item;
    }
    void push(const T&);
    void pop();
    T& peek() const;
    ~Stack() {
        while (!empty()) {
            pop();
        }
    }
private:
    struct Node {
        T item;
        Node* next;
        Node(const T& t, Node* link) :item{t}, next{link} { }
    };
    Node* first;
    stacksize n;
};

template<class T>
Stack<T>::Stack(const Stack& s) :first{nullptr}, n{0}{
    for (auto t = s.first; t != nullptr; t = t->next) {
        push(t->item);
    }
}

template<class T>
Stack<T>& Stack<T>::operator=(Stack s) {
    std::swap(first,s.first);
    std::swap(n,s.n);
    return *this;
}

template<class T>
Stack<T>::Stack(Stack&& s) :first{s.first}, n{s.n} {
    s.first = nullptr;
    s.n = 0;
}

template<class T>
Stack<T>& Stack<T>::operator=(Stack&& s) {
    std::swap(s.n,n);
    std::swap(s.first,first);
    return *this;
}

template<class T>
void Stack<T>::push(const T& t) {
    first = new Node{t,first};
    ++n;
}

template<class T>
void Stack<T>::pop() {
    Node* oldfirst = first;
    first = first->next;
    delete oldfirst;
    --n;
}

template<class T>
T& Stack<T>::peek() const {
    return first->item;
}

Answer 1

Code Review

I find your placement of & inconsistent with *

    Node(const T & t, Node* link) :item{t}, next{link} { }

And r-value reference declaration stranger still

    Stack(Stack & & );  // I am surprised that compiled. As each `&` is a separate token
                        // While `&&` is a single token.

This is all part of the type information. Put it with the type.

    Node(const T& t, Node* link) :item{t}, next{link} { }
    Stack(Stack&& rhs);

You provide a push by copy.

void push(const T & );

and you are familiar with move semantics. Why not provide a push by move?

void push(T&& val);

Very Minor personal preference.

Personally I prefer const on the right of the type. The rule is that const binds left unless it is on the very left hand side then it binds right. There is one obsecure corner case were this makes a difference. But it is obscure so don't worry.

    Node(T const& t, Node* link) :item{t}, next{link} { }

I just find it easier when reading types (as you read them right to left).

    char const * const  x;
    // x is "const pointer" to a "const char"

    char const *   x;
    // x is "pointer" to a "const char"

    char * const  x;
    // x is "const pointer" to a "char"

Answer 2

Seeing as you need to make a new stacksize object in the operator[] function, I wouldn't pass the parameter i as a const reference. This way you can use the copied parameter as part of the logic and shorten the code while still not effecting the caller. I would also add error checking here to see if the index is out of range and either throw a std::out_of_range exception or return a dummy value

#include <stdexcept>

T& operator[](stacksize i) const{
    if(i < 0 || i >= n) throw std::out_of_range("You dun goofed the range");
    Node* traverse = first;
    while (i--) traverse = traverse->next;
    return traverse->item;
}

You should also be doing this type of error checking in your other functions

Answer 3

These are some thoughts/questions from a C++ noob trying to learn from your code.

Const correctness

• Shouldn't your peek function return a const reference? Right now it returns a reference, but peek isn't aptly named if it returns something you expect to be able to play with. I think this is especially important since you declare peek const but a const function that returns something to a client that the client can change is not so much a classic const function

Move semantics

• Can't you use move to be explicit here:

  template<class T> Stack<T>::Stack(Stack&& s) :first{s.first}, n{s.n} { s.first = nullptr; s.n = 0;

  Why not just std::move? You're basically implementing the default move operator, no? Or if not, surely you can implement this template function above in terms of the one that comes after it, the operator=? This would cut down on code redundancy.

Error checking

• Right now your pop() function doesn't seem to have any error checking or special reaction if a client seeks to pop off an empty list.