Follow up: Implementation of a generic Stack in C++

Question

This question is a follow up to the question posted here.

From the answer in that question, I've improved my implementation and would like another code review. I've made a myriad changes including but not limited to the use of unique pointers, adding constructors for Node and fixing the destructor.

I would still like the focus to be on those 4 points, and hope that my improvements have implemented those 4 points better.

1. My Stack class guarantees Strong Exception Safety, using the copy and swap idiom

2. The container still works if T passed in is not default constructible (Please give an example on how to address this problem if it still doesn't work in this implementation)

3. Correctness of the implementation of the Rule of Five

4. General implementation correctness and efficiency (i.e: No memory leaks, dangling pointers ... ...)

Stack Code below:

#pragma once
#include <memory>
template <typename T>
class Stack
{
 public:
 Stack();
 Stack(const Stack& other);
 Stack(Stack&& other) noexcept;
 ~Stack();
 Stack<T>& operator =(const Stack<T>& other);
 Stack<T>& operator =(Stack<T>&& other) noexcept;
 void swap(Stack<T>& other) noexcept;
 friend void swapStacks(Stack<T>& A, Stack<T>& B)
 {
 A.swap(B);
 }
 void pop();
 T& top();
 void push(const T& item);
 void push(T&& item);
 private:
 struct Node
 {
 Node* next;
 T data;
 Node()
 :next(nullptr)
 , data(0)
 {}
 Node(const T& item, Node* next)
 :next(next)
 , data(item)
 {}
 Node(T&& item, Node* next)
 :next(next)
 , data(std::forward<T>(item))
 {}
 };
 Node* head;
};
template <typename T>
Stack<T>::Stack()
 :head(nullptr)
{}
template <typename T>
Stack<T>::Stack(const Stack& other)
 : Stack()
{
 Node** headptr = &head;
 for (Node* curr = other.head; curr != nullptr; curr = curr->next)
 {
 std::unique_ptr<Node> tmp(std::make_unique<Node>(curr->data, curr->next));
 *headptr = tmp.release();
 headptr = &(*headptr)->next;
 }
}
template <typename T>
Stack<T>::Stack(Stack&& other) noexcept
 :Stack()
{
 swapStacks(*this, other);
}
template <typename T>
Stack<T>::~Stack()
{
 while (head != nullptr)
 pop();
}
template <typename T>
Stack<T>& Stack<T>::operator =(const Stack<T> &other)
{
 Stack tmp(other);
 swapStacks(*this, tmp);
 return *this;
}
template <typename T>
Stack<T>& Stack<T>::operator =(Stack<T>&& other) noexcept
{
 swap(*this, tmp);
 return *this;
}
template <typename T>
void Stack<T>::swap(Stack& other) noexcept
{
 using std::swap;
 swap(head, other.head);
}
template <typename T>
void Stack<T>::pop()
{
 if (head == nullptr)
 throw std::runtime_error("No item found in stack");
 Node* curr = head;
 head = head->next;
 delete curr;
}
template <typename T>
void Stack<T>::push(const T& item)
{
 std::unique_ptr<Node> tmp(std::make_unique<Node>(item, head));
 head = tmp.release();
}
template <typename T>
void Stack<T>::push(T&& item)
{
 std::unique_ptr<Node> tmp(std::make_unique<Node>(std::move(item), head));
 head = tmp.release();
}
template <typename T>
T& Stack<T>::top()
{
 if (head == nullptr)
 throw std::runtime_error("No item found in stack");
 return head->data;
}

To test the Stack, I've also created a TestObject that has no default constructor:

#pragma once
class TestObject
{
 int testVal;
 public:
 TestObject(int i)
 :testVal(i)
 {}
 int getTestVal();
};
int TestObject::getTestVal()
{
 return testVal;
}

And here are my tests:

#include "Stack.h"
#include "TestObject.h"
#include <iostream>
void testStack();
int main()
{
 testStack();
 system("pause");
 return 0;
}
void testStack()
{
 Stack<int> testStack;
 testStack.push(5);
 std::cout << testStack.top() << "\n";
 testStack.push(7);
 std::cout << testStack.top() << "\n";
 testStack.pop();
 std::cout << testStack.top() << "\n";
 Stack<TestObject> dummy;
 dummy.push(TestObject(3));
 dummy.push(TestObject(4));
 Stack<TestObject> testStack2(dummy);
 std::cout << testStack2.top().getTestVal() << "\n";
 testStack2.pop();
 std::cout << testStack2.top().getTestVal() << "\n";
 Stack<TestObject> testStack3;
 testStack3 = testStack2;
 testStack3.push(TestObject(5));
 testStack3.push(TestObject(6));
 std::cout << testStack3.top().getTestVal() << "\n";
 testStack3.pop();
 std::cout << testStack3.top().getTestVal() << "\n";
 testStack3.pop();
 std::cout << testStack3.top().getTestVal() << "\n";
}

Here is the output I got, which is correct:

Test output and result

EDIT: I've made a bit of change to the copy constructor because I didn't quite understand the implementation that I had above (It was from an answer on the earlier question). Please let me know what you think of this implementation vs the other one.

template <typename T>
Stack<T>::Stack(const Stack& other)
 : Stack()
{
 if (other.head == nullptr)
 {
 head = nullptr;
 }
 else
 {
 Node* curr = other.head;
 std::unique_ptr<Node>headCpy(std::make_unique<Node>(curr->data, nullptr));
 head = headCpy.release();
 Node* tmp = head;
 while (curr->next != nullptr)
 {
 curr = curr->next;
 std::unique_ptr<Node>tmp2(std::make_unique<Node>(curr->data, nullptr));
 head->next = tmp2.release();
 head = head->next;
 }
 head = tmp;
 }
}

Answer 1

Smart Pointers

You seem to be missing the point on smart pointers. The benefit of using unique_ptr is that they help with memory management and clean up after themselves. The way you're using it:

std::unique_ptr<Node> tmp(std::make_unique<Node>(item, head));
head = tmp.release();

Is the same as doing this:

head = new Node(item, head);

When you call release, the unique_ptr stops looking after the memory, which is why you're having to explicitly call delete curr; in your pop method.

It may be worth you reviewing how this answer uses unique_ptr in their stack implementation.

• Mantracker: I'm using head not as a smart pointer, but the tmp variable is a smart pointer. head is still of type Node*. I made tmp unique pointer so that I don't have to call delete tmp later on. Although it would probably be better that I use head as a unique ptr too
• forsvarir: @Mantracker When you call release on the unique_ptr it's state becomes empty, there's nothing for it to release. The only benefit you have from it being a smart pointer is that delete would be called automatically if an exception was thrown between the construction and the release. Since you're always releasing the line after creation, this seems quite unlikely.
• Mantracker: I see. The thing was that in my previous implementation, new Node can throw an exception if T is not default constructible, which is why I had to use a smart pointer. Otherwise I would have to catch that exception and call delete in the catch block to prevent memory leak
• forsvarir: @Mantracker Maybe I'm missing something, but I don't see how that works. If new Node throws an exception, tmp would never be assigned a value, it will either be nullptr in which case delete doesn't do anything, or it will be unitialized in which case delete is probably going to blow up.
• forsvarir: @Mantracker OK, so now that I've looked at your previous code and the answer, I think I understand. The important difference between your current code and the previous version is that rather than creating Node and then updating it(the previous version), you now use a custom constructor on node so that the creation and initialisation is atomic (it either succeeds and assigns the value, or fails and doesn't assign the value and cleans up itself).