C++ Stack implementation with linked-list It's proving difficult to find implementations of a Stack with linked list and Im trying to figure out if my implementation is correct/not missing anything obvious.

template &lt;class T&gt;
class Stack {
private:
  struct node {
    T data;
    node* next = nullptr;
    node(T data, node* next) : data(data), next(next) {};
  };
  node* top_ = nullptr;
public:
  Stack() {}
  Stack(T seed[], size_t len) {
    for(size_t i = 0; i &lt; len; ++i) {
      Push(seed[i]);
    }
  }
  void Push(T value) {
    node* ele = new node { value, top_ };
    top_ = ele;
  }
  bool IsEmpty() {
    return top_ == nullptr;
  }
  T Peek() {
    if (IsEmpty())
      throw std::runtime_error("Stack is empty");
    return top_-&gt;data;
  }
  T Pop() {
    if (IsEmpty())
      throw std::runtime_error("Stack is empty");
    node* popped = top_;
    T result = popped-&gt;data;
    top_ = popped-&gt;next;
    delete popped;
    return result;
  }
};


My tests (sample basic ones below) do show working functionality as well, so its really just my code structure/procedures I'm worried about

int seed[3] { 5,4,3 };
Stack&lt;int&gt; stack { seed, 3 };
REQUIRE( stack.Pop() == 3 );
REQUIRE( stack.Pop() == 4 );
REQUIRE( stack.Pop() == 5 );
REQUIRE_THROWS_WITH( stack.Pop(), "Stack is empty" );
REQUIRE_NOTHROW( stack.Push(9) );
REQUIRE_NOTHROW( stack.Push(3) );
REQUIRE( stack.Peek() == 3 );
REQUIRE( stack.Pop() == 3 );
This is the archaic form of template declaration:

template &lt;class T&gt;


The more modern form is:

template&lt;typename T&gt;


Technically not difference but class implies user defined type while typename implies any type. Most modern libraries will use the newer version. There is also one obscure corner case template templates were it makes a difference.

You don't need to define a constructor for node.

  struct node {
    T data;
    node* next = nullptr;
    node(T data, node* next) : data(data), next(next) {};
  };


The initializer list contruct will have the same affect.

  struct node {
    T data;
    node* next;
  };
  ...
  new node {data, top};


Also I would use standard naming convention. User defined types start with an upper case letter while variables and methods start with a lower case letter. This helps you distinguish between object creation and function calls very easily.

You have missed the destructor. This means a non empty stack will leak all its members when it goes out of scope. So you should define a destructor to cleanup when the object is destoryed.

Now that you have a destructor:

You don't obey the rule of 3 or 5 (somebody else mentioned the rule of 7 but I don't know what that is, if somebody would like to clarify in the comments?).

Rule of 3: If you define a destructor, copy constructor or copy assignment then you probably need all three.

Rule of 5: Rule of 3 + the 2 move operators. Allows you define move operators for your object so that it is move compatible.

The alternative to using the Rule of 3/5 is to use the Rule of 0. This basically means the node object cleans itself up. This would require using std::unique_ptr. The top in your list and the next in the node would need to use this type.

Personally I don't think this is appropriate for building lists (but it is definitely an option and other people would recommend it).

You are passing by value here:

  void Push(T value)


This is fine for simple types of T. But if T is expensive to copy then you copy it to the Push method. Then you copy it again when you construct the object.

Try pushing by reference and moveable reference.

 void Push(T const&amp; value);  // Pass value by reference.
 void Push(T&amp;&amp; value);       // Pass by r-value ref allowing a move.


This function does not change the state of the object.

  bool IsEmpty() {
    return top_ == nullptr;
  }


You should probably mark it as const. This allows you to pass the stack by const reference and still check the state of the object.

DRY you code.
The following code is repeated in two places

    if (IsEmpty())
      throw std::runtime_error("Stack is empty");


That means it is a candidate to be put into its own function.

You can not implement Pop() that returns a value in an exception safe manner. As such the standard stack uses two separate methods top() which returns a reference to the top value and pop() that returns void but removes the top item from the stack.

You should probably follow this convention.

These two functions return by value.

  T Peek()
  T Pop()


This means you make a copy of the value. This is fine for simple types like integer but for complex classes this is potentially expensive. Another reason to use top() and void pop(). But here at least Peek() should return a reference to avoid the copy.