Stack implementation using linked list in C++

Question

I'd like some feedback for the code I wrote. I haven't programmed in C++ for two years.

#include <string>

#pragma once
template <class T>
class StackLinkedList
{
public:
    StackLinkedList() : m_List(nullptr), m_Size(0) {}
    ~StackLinkedList();
    void push(const T& val);
    void pop();
    T& top();
    const T& top() const;
    size_t size() const;
    bool empty() const;
private:
    struct Node;
    Node *m_List;
    size_t m_Size;
};

template <class T>
struct StackLinkedList<T>::Node
{
    T m_Val;
    Node* m_Next;
    Node(const T& val, Node* nextPtr) : m_Val(val), m_Next(nextPtr) {}
};

template<class T>
StackLinkedList<T>::~StackLinkedList()
{
    typename Node* nextNodePtr;
    for (typename Node* curNodePtr = m_List; curNodePtr != nullptr; )
    {
        nextNodePtr = curNodePtr->m_Next;
        delete curNodePtr;
        curNodePtr = nextNodePtr;
    }
}

template<class T>
void StackLinkedList<T>::push(const T& val)
{
    typename Node* newNode = new Node(val, m_List);
    m_List = newNode;
    m_Size++;
}

template<class T>
void StackLinkedList<T>::pop()
{
    if (m_List != nullptr)
    {
        typename Node* curNodePtr = m_List;
        m_List = m_List->m_Next;
        delete curNodePtr;
        m_Size--;
    }
}

template<class T>
T& StackLinkedList<T>::top()
{
    if (empty())
    {
        throw new std::string("Stack is empty");
    }
    return m_List->m_Val;
}

template<class T>
const T& StackLinkedList<T>::top() const
{
    if (empty())
    {
        throw new std::string("Stack is empty")
    }
    return m_List->m_Val;
}

template<class T>
size_t StackLinkedList<T>::size() const
{
    return m_Size;
}

template<class T>
bool StackLinkedList<T>::empty() const
{
    return m_Size == 0;
}

Answer 1

This is quite good, actually. I have one big comment, and a few relatively minor comments.

RULE OF THREE. Anytime you define a destructor, you want to make sure you define your copy constructor and copy assignment operators. The reason for that is the following:

StackLinkedList<T> stack;
stack.push(...);
stack.push(...);
{
    StackLinkedList<T> stack2 = stack;
    stack2.push(...);
    // <== stack2 deleted here
}
// <== uh-oh, stack2 deleted all the nodes, but stack still thinks
// it has valid objects that it's holding onto that it needs to delete too!

Your class is sorely missing:

StackLinkedList(const StackLinkedList& );

friend void swap(StackLinkedList&, StackLinkedList& );
StackLinkedList& operator=(StackLinkedList other) {
    swap(*this, other);
    return *this;
}

For more on why I suggest this way specifically, see what is the copy-and-swap idiom.

You didn't mention if you were using C++11 or not. If you are, you want the Rule of Five: you will also need to implement the move constructor and move assignment operator. For this class in particular, those two should be very short.

---

Other comments are minor.

Use of typename. There are places where you need to use typename and there are places where you don't. Just don't use it in those latter cases - it's just unnecessary. In your code, literally every usage of typename is redundant and can be omitted.

top(). The typical implementation of containers is to make operations as fast as possible. So for top(), that would mean not checking empty() - that's up to the user. Just simply return m_List->m_Val and note that it's just undefined behavior to call top() on an empty stack. If you disagree and want to throw an exception, that is of course perfectly reasonable as well - but this not the way to do it:

throw new std::string("Stack is empty");

You're throwing a std::string*. Anybody that catches it would have to delete it, otherwise you're leaking memory - and std::string* doesn't really convey information. If you look in <stdexcept>, there's a better choice of exception type: std::out_of_range. Standard practice is to throw by value, catch by reference. So that line would be:

throw std::out_of_range("Stack is empty");

#pragma once should be the first line in your file, you have it after a different #include.