c++ destructor in linked lists

Question

I Have built a stack using linked lists in c++, but I was being told that the whole thing is worthless since my destructor is not doing its job efficiently , so can anybody tell me how can I build a destructor for this code. It was my understanding that a constructor work is to delete the memory that is unused, is there any other function of the destructor that that I mentioned ? any help would be appreciated.

#include <iostream>
#include <string>
#include <limits>
using namespace std;

class Stack
{
private:
    struct node
    {
        int data;
        node* next;

        node()
        {
          int data = 0;
          next = nullptr;
        }

    };
node* top;
public:
    Stack()
    {
        top = nullptr;
    }
    ~Stack()
    {
       delete top;
    }
 void push(int n)
 {
    node* temp = new node;
    temp->data = n;
    temp->next = top;
    top = temp; 
 }

    int pop()
    {
        node* temp = top;
        top = top->next;
        return temp->data;
    }

    int peek()
    {
        return top-> data;
    }

    bool isEmpty()
    {
        return top == 0;
    }
};

int main()
{
    Stack stack;

    std::string command;

    while (true)
    {
        std::cout << "stack>";
        std::cin >> command;
        try
        {
            if (command == "pop")
            {
                if (stack.isEmpty())
                {
                    throw std::runtime_error("error: stack is empty");
                }
                 std::cout << stack.pop() << std::endl;
            }

            else if (command == "push")
            {
                int n;
                if (!(std::cin >> n))
                {
                    throw std::runtime_error("error: not a number");
                }
                stack.push(n);
            }

            else if (command == "peek")
            {
                if (stack.isEmpty())
                {
                     throw std::runtime_error("error: stack is empty");
                }
                std::cout << stack.peek() << std::endl;
            }

             else if (command == "end")
            {
                 while (!(stack.isEmpty()))
                 {
                    std::cout << stack.pop() << std::endl;
                 }
                return 0;
            }
            else
            {
                throw std::runtime_error("error: invalid command");
            }
        }
        catch (std::runtime_error& e)
        {
            std::cin.clear();
            std::cin.ignore(numeric_limits<streamsize>::max(), '\n');
            std::cerr << std::endl << e.what() << std::endl;
        }
    }
    return 0;
}

Answer 1

You have memory leak in Stack::pop() method and your Stack() destructor only deletes top so if you have more than 1 elements you would have yet another leak. When you deal with dynamically allocated memory you better use smart pointers:

class Stack
{
    struct node;
    using node_ptr = std::unique_ptr<node>;
private:
    struct node
    {
        int data;
        node_ptr next;

        node(int n, node_ptr nxt) : data(n), next( std::move(nxt) )
        { // your old ctor has a bug, you "initialized" local variable
        }
    };

    node_ptr top;
    Stack() = default;
    ~Stack()
    {
         while(top)
             top = std::move( top->next );
    }

    int pop()
    {
       node_ptr temp = std::move(top);
       top = std::move(temp->next);
       return temp->data;
    }

    void push(int n)
    {
        top = std::make_unique<node>(n,std::move(top));
    }

    int peek() const
    {
        return top->data;
    }

    bool isEmpty() const
    {
       return !top;
    }
};

your code becomes smaller and easier to understand and free from leaks you had before. You do not have to provide explicit Stack destructor, but it would be inefficient (doing recursive call but still correct). Provided destructor just more efficient.

Answer 2

You have a list like this:

top---->node->node->node->node->...

In the destructor of Stack, you delete the object pointed by top. What you're left with is:

              node->node->node->...

Notice that most of the nodes have not been deleted, and since nothing points to them anymore, there is no longer a way to delete them. This is called a memory leak.

To fix the destructor, you must delete each node on the list. Hint: Use a loop; make a copy of the next pointer, delete the current node and move on to the next one.

---

The destructor is not the only place where the stack leaks. pop fails to delete the top node before unlinking it from the list.

Furthermore, assigning or coping a Stack object will cause the both objects to have the same list and consequently attempt to delete it twice (once in each of the respective object's destructor). The behaviour will be undefined. Whenever you manage resources in the destructor, you always need to define a copy (and move) constructor and assignment operators that manage the resources also. This is called the rule of 3 (5 for move operations). You can declare them deleted if you don't want the class to be copyable (or movable).

It is a bad design to have owning bare pointers. A better design is to use std::unique_ptr

Your constructor of node leaves data member uninitialized. I don't think your stack ever reads a default initialized node, so that might not be a problem, but you may have different expectations for the constructor that you wrote.

Answer 3

You need to deallocate memory for while link list :

~Stack()
    {
        while(top)
        {
            struct node* curr = top;
            top = top->next;
            delete curr;
        }
        top = nullptr;
    }

Answer 4

You are deleting only the top element.

Try adding ~node() { delete next; } to your node struct.

And make sure that it's NULL-terminated, so that it halts!