prevent memory leak c++

Question

How to prevent memory leaks in the below code? I added "delete sentinel" before return statement. But how to deal with the memory leak of these 2 lines d->next = new ListNode(sum % 10) and d->next = new ListNode(1)

ListNode* addTwoNumbers(ListNode* l1, ListNode* l2) {
        ListNode* c1 = l1;
        ListNode* c2 = l2;
        ListNode* sentinel = new ListNode(0);
        ListNode* d = sentinel;
        int sum = 0;

        while(c1 != NULL || c2 != NULL) {
            sum /=10;

            if(c1 != NULL){
                sum += c1->val;
                c1 = c1->next;
            }

            if(c2!= NULL){
                sum += c2->val;
                c2 = c2->next;
            }

            d->next = new ListNode(sum % 10);
            d = d->next;
        }    

        if(sum /10 == 1){
                d->next = new ListNode(1);
        }

        return sentinel->next;
    }
};

Short answer is that you need some deletes to go with your news. On the surface, it would seem like you'd need to delete d->prev, but I'm not sure what your ListNode implementation looks like. — Kevin K, Aug 29 '17 at 19:57
@Ron, this makes me curious to know the reason given and what industry / size of company you're working in, because where I work smart pointers are allowed. (And it's the first time I hear somebody say that) — alain, Aug 29 '17 at 20:04
@alain I was talking about the college / school practices when giving assignments not the industry. If they knew what they were doing they would not insist on class _wiring_, multiple inheritance and diamond problem. More often than not what they teach is limited to trivial numerical analysis and the above _data structures_. Constraints such as: "you are not allowed to use smart pointers" are frequent. — Ron, Aug 29 '17 at 20:09
Ah now I understand... thanks for the response. I can see some sense in teaching these things, but only if they also teach the modern tools. — alain, Aug 29 '17 at 20:13
@alain The bad thing is they usually teach them to do manual memory management 1st, and the way it really should be done later. So most people coming from the uni to work in the industry have to be brainwashed about the usage of `new` / `delete` before you can unleash them onto production code. — user0042, Aug 29 '17 at 20:17
I guess smart pointers are more appreciated when the tediousness of new/delete has been experienced first-hand :-) — alain, Aug 29 '17 at 20:24
Just use smart pointers ([std::unique_ptr](http://en.cppreference.com/w/cpp/memory/unique_ptr), [std::shared_ptr](http://en.cppreference.com/w/cpp/memory/shared_ptr) & [std::weak_ptr](http://en.cppreference.com/w/cpp/memory/weak_ptr)) already... — Jesper Juhl, Aug 29 '17 at 21:11
What gets me is that there are tons of resources out there, (SO, videos, even talks by the inventor of C++ himself extolling the teaching of C++ the modern way), yet the schools don't budge one bit from their obsolete ways of teaching the language. — PaulMcKenzie, Aug 29 '17 at 21:17

Artemy Vysotsky · Answer 1 · 2017-08-29T20:18:12.827

There is a straightforward answer - the current code has one leak - sentinel, all other allocated nodes are not leaked - since they are still could be accessed through returned pointer sentinel->next

So short version to fix the leak would be to change

    return sentinel->next;

with

    d = sentinel->next;
    delete sentinel;
    return d;

Longer version will require to change the usage of raw pointers to smart pointers std::unique_ptr in this case looks like pointer of the choice. But this should be done for the whole program not only one function.

Actually - as afterthought - even more desirable solution would be to use std::forward_list instead of homemade list

Michaël Roy · Accepted Answer · 2017-08-29T21:29:09.557

Your code is really confusing... Not because the algorithm is complex, but because you use several variables to point to the same thing. Why this multitude of unused variables ?

ListNode* addTwoNumbers(ListNode* l1, ListNode* l2) 
{
    ListNode* sentinel = new ListNode(0);
    listNode* d = sentinel;
    int sum = 0;
    while (l1 != NULL || l2 != NULL) // use pointers l1 and l2, since passed by value.
    {
        if (l1 != NULL)
        {
            sum += l1->val;
            l1 = l1->next;
        }

        if (l2 != NULL) 
        {
            sum += l2->val;
            l2 = l2->next;
        }

        d->next = new ListNode(sum % 10);
        sum /= 10;
        d = d->next;
    }    

    if (sum)
    {
        d->next = new ListNode(sum);
    }

    // And this is where one of your memory leaks was...
    if (sentinel->next)
    {
        d = sentinel->next;      // delete useless sentinel.
        sentinel->next = NULL;   // important !  see ~ListNode()
        delete sentinel;
        sentinel = d;
    }
    return sentinel;   // and not result->next.
}

To avoid leakage from a list, you need to write a proper list destructor to free child nodes and other dependent resources, if any. This is a rather recurrent theme in c++.

ListNode::~ListNode()
{
    // never destroy lists recursively !!  Destroying a large list
    // that way could bust the stack, hence this odd looking loop.
    for (ListNode * p = next, * q = NULL; p != NULL; p = q)
    {
        q = p->next;
        p->next = NULL;   // <-- avoids recursion.
        delete p;
    }
}

score 0 · Answer 3 · answered Aug 29 '17 at 20:28

0

Don't use raw pointers. See the C++ Core Guidelines (state of the art guidance from some of the foremost leaders in C++).

answered Aug 29 '17 at 20:28

Paul Floyd

5,530
5
29
43

prevent memory leak c++

3 Answers3