I want to know why am i getting an error when i delete temp in this function.

`

#include<iostream>
using namespace std;
//NODE FOR LINKED LIST
class node {
public:
    int data;
    node* next;
    node(int val) {
        data = val;
        next = NULL;
    }
    ~node() {
        delete next;
    }
};
//Linked List
class LinkedList {
    node* head;
public:
    LinkedList() {
        head = NULL;
    }
    //checking if head is Null or not
    void insertAtEnd(int val) {
        if (head == NULL) {
            head = new node(val);
            return;
        }

        node* temp = head;
        while (temp->next != NULL)
            temp = temp->next;
        temp->next = new node(val);
        //delete temp;
        //Now if i use this delete temp my loop in display function breaks and runs indefinitely
    }
    void display() {
        cout << "Your List : ";
        node* temp = head;
        while (temp != NULL) {
            cout << temp->data << ">";
            temp = temp->next;
        }
        cout << endl;
        delete temp;
    }
};

int main() {

    LinkedList obj;
    //sample tests
    obj.insertAtEnd(10);
    obj.insertAtEnd(20);
    obj.insertAtEnd(30);
    obj.insertAtEnd(40);

    obj.display();
    system("pause");
    return 0;
}

`

I tried commenting this delete out and it worked but its been leaking memory a simple solution i thought was to make a *temp in constructor then deleteing it in destruction assigning NULL in every function that needs it.

CodePudding user response：

You need to understand the ownership of the pointers in your code:

• each node is owned by it's predecessor

• except for the head node, which is owned by the LinkedList itself

temp, however, is never used as a pointer with ownership, so deleting it is conceptionally wrong (in insertAtEnd as well as in display). Technically explained, in your loop, temp will point to the predecessor of the last node added, which is still part of the list. When you now delete temp, it's predecessor's next pointer will still point to the node you just deleted.

To avoid the memory leak, you need to add a destructor to LinkedList and delete head. That will destruct the whole chain recursively.

This solution would work, but is stil not ideal, since now node will delete something it did not construct, hence the memory management is asymmmetric. Better remove the delete operation from the nodes destructor and let LinkedList` do the creation as well as the deletion.

CodePudding user response：

Your LinkedList class should handle the deletion of nodes. Generally, you should strive to encapsulate the management of memory in the owner of the data structure, in this case, the LinkedList class owns the node class. The node class does not explicitly allocate any memory, and thus shouldn't handle the deletion of anything.

Your LinkedList destructor should look something like this:

~LinkedList() {
        node* deleter_node = nullptr; //pointer to current node (node we want to delete)
        deleter_node = head; //deleter points to the head of your list

        while (deleter_node != nullptr) { //traverse list and delete nodes
            node* temp = nullptr;
            temp = deleter_node -> next; //temp ensures we 
                                         //don't loose the pointer to the next node

            delete deleter_node; //delete current node
            deleter_node = temp; 
        }
}

To make ownership more explicit, consider rewriting LinkedList with the node structure as a public member.

Something like this:

class LinkedList {
    Node* head;
public:
    struct Node { //node is now a public member struct
        int data = 0;
        Node* next = nullptr;
    };

    LinkedList() { //default constructor
        head = nullptr;
    }
    ~LinkedList() { //destructor handles memory management
        Node* deleter_node = nullptr;
        deleter_node = head;
        while (deleter_node != nullptr) {
            Node* temp = nullptr;
            temp = deleter_node -> next;
            delete deleter_node;
            deleter_node = temp;
        }
    };