C++: potential memory leak in linked list

Question

I'm writing a class of linked list, I feel that for the member function that used to delete specific element might cause the memory leak. The code is below.

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

class linked_list
{
private:
    node *head,*tail;
public:
    linked_list()
    {
        head = NULL;
        tail = NULL;
    }
    void add_node(int n)
    {
        node *tmp = new node;
        tmp->data = n;
        tmp->next = NULL;

        if(head == NULL)
        {
            head = tmp;
            tail = tmp;
        }
        else
        {
            tail->next = tmp;
            tail = tail->next;
        }
        
    }
    void DelElem(int locat)
    {
        int j{1};
        node* tmp = new node;     
        if (locat == 1)
        {      
            tmp  = head->next;
            head = tmp;
            delete tmp;
        }
        else
        {
            node* n = head;
            while (j < locat - 1)
            {
                n = n->next;
                j++;
            }
            tmp = n->next;
            n->next = tmp->next;
            delete tmp; 
        } 
    }

For function 'DelElem', I firstly created a pointer tmp by new operator. However, I assign different address for it which means I lost the original one at the initialization.

How can I fix this problem?

Why are you using new in a function that removes nodes? You should never do so. — drescherjm, Feb 17 '21 at 00:58
***I firstly created a pointer tmp by new operator*** This is wrong. You don't use new to create a pointer. You use new to allocate a new node. `node* tmp;` creates a pointer. c++ is not a language where you use new to create every variable. — drescherjm, Feb 17 '21 at 00:59
***However, I assign different address for it which means I lost the original one at the initialization.*** That is correct, you are creating a memory leak with the unnecessary `new` which allocates a new node. — drescherjm, Feb 17 '21 at 01:03
*"How can I fix this problem?"* -- by not creating the problem in the first place. You correctly identified the problem, so get rid of it. — JaMiT, Feb 17 '21 at 01:10
Food for thought: [Declaring variables inside loops, good practice or bad practice?](https://stackoverflow.com/questions/7959573/declaring-variables-inside-loops-good-practice-or-bad-practice) (The principles apply to `if` statements as well as to loops.) — JaMiT, Feb 17 '21 at 01:15
@TedLyngmo Hi, I just saw the answer and I accepted yours. sry bro. — Nicolas H, Feb 17 '21 at 15:08

Abhishek Dutt · Accepted Answer · 2021-02-17T03:18:44.340

There are few issues with your instance of code, I have corrected that:-

As pointed by others, you are not required to use `new` keyword to declare a pointer.
When one tries to delete the first node of the linked list, then according to your code, it will delete the second node, because of the following
```
tmp  = head->next;
head = tmp;
delete tmp;
```
Here, tmp is initially pointing to second node,because head->next refers to 2nd node. So instead of that, it should have been like this:-
```
tmp = head;
head = head->next;
delete tmp;
```
Now, tmp will point to 1st node, in second line, head will point to 2nd node, and then the first node, pointed by tmp gets deleted.

Here is the corrected version of code:-

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

class linked_list {
private:
    node *head, *tail;

public:
    linked_list()
    {
        head = NULL;
        tail = NULL;
    }
    void add_node(int n)
    {
        node* tmp = new node;
        tmp->data = n;
        tmp->next = NULL;

        if (head == NULL) {
            head = tmp;
            tail = tmp;
        }
        else {
            tail->next = tmp;
            tail = tail->next;
        }
    }
    void DelElem(int locat)
    {
        int j{ 1 };
        node* tmp;
        if (locat == 1) {
            tmp = head;
            head = head->next;
            delete tmp;
        }
        else {
            node* n = head;
            while (j < (locat - 1)) {
                n = n->next;
                j++;
            }
            tmp = n->next;
            n->next = tmp->next;
            cout << tmp->data;
            delete tmp;
        }
    }
};

C++: potential memory leak in linked list

1 Answers1