C++ SLList - Delete temporary node from heap?

I'm learning linked lists and I thought the rule was every new has to be answered with a delete to clear the dynamically allocated memory. Here is my code where I create 3 nodes in a list, each time creating a temp node with Node *temp = new Node; in the createNode function. My question is, isn't this dynamically allocating on the heap and should I need to delete temp;? Without it my code runs fine, but when I add delete temp; at the end of the function I get a read access violation error. Shouldn't it not make a difference anyway as temp is now no longer being used after the function ends? Hope that makes sense, thanks for your help.

#include <iostream>
#define Log(x) std::cout << x << std::endl;

struct Node
{
    int data;
    Node *next;
};

class SLList
{
private:
    Node *head;
    Node *tail;

public:
    SLList() 
    {
        head = NULL;
        tail = NULL;
    }

    void createNode(int value)
    {
        Node *temp = new Node;
        temp->data = value;
        temp->next = NULL;

        if (head == NULL)
        {
            head = temp;
            tail = temp;
        } 
        else
        {
            tail->next = temp;
            tail = temp;
        }
    }

    void printList()
    {
        Node *temp;
        temp = head;
        while (temp != NULL)
        {
            Log(temp->data);
            temp = temp->next;
        }
    }
};

int main()
{
    SLList list1;
    list1.createNode(5);
    list1.createNode(7);
    list1.createNode(2);

    list1.printList();

    std::cin.get();
}

2 answers

  • answered 2018-11-07 22:32 Kirill Voroshilov

    It's true that you should free dynamically allocated memory, but you try to do it at wrong time.

    When you call Node *temp = new Node; you do not create temporary node, you create node on heap and make a temporary pointer to it on stack. After you've added that node to the list you don't need that pointer anymore (its value is stored in your SList object), but you still need the object it points to.

    So:

    • Yes, you should free your memory by calling delete
    • No, you must not do it inside createNode method because you still need this data. Free your memory when you don't need it: when you remove element from the list.

  • answered 2018-11-07 22:40 Kuba Ober

    A delete for every new is correct, but doesn’t tell you when is it appropriate to delete. You cannot delete while you have pointer(s) to the data and you intend to use those pointer values later to dereference them.

    The pointers to the nodes live within the list, and in the head pointer, so you cannot delete those nodes yet. You can only do so when whatever pointers there were to any given node will be cleared or otherwise left un-dereferenced prior to assigning a new value to them.