5

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; } };
Improve this question
8
  • 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. Commented Aug 29, 2017 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) Commented Aug 29, 2017 at 20:04
  • 1
    @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. Commented Aug 29, 2017 at 20:09
  • 2
    @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. Commented Aug 29, 2017 at 20:17
  • 1
    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. Commented Aug 29, 2017 at 21:17

3 Answers 3

Reset to default
1

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; } }
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

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

Improve this answer

Comments

0

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

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.