14

Is the following C++ code a memory leak?

list.push_back(new String("hi"));

As I understand it, push_back from any std collection/container always makes a copy. So if the new string is copied, nothing can ever delete the new'd string right? since there is no reference to it after the push_back...

Am I correct or wrong here?

Thanks.

Jbu

edit: I think I am wrong, since new will return a pointer...we'll always have the pointer to be able to delete the new String

jbu
  • 15,831
  • 29
  • 82
  • 105
  • 1
    It depends on the definition of `list`. – Steve Townsend Nov 24 '10 at 20:23
  • 3
    Depends what `list` is. Assuming it's a `std::list`, @UncleBens is correct: it may still be possible to clean up correctly most of the time. But you must do that cleanup manually; `std::list` will not do it for you. – aschepler Nov 24 '10 at 20:24
  • Yes. Because the variable 'list' is actually of a type that has a function called 'push_back' with an empty body. – Edward Strange Nov 24 '10 at 20:37

7 Answers7

7

Yes, but not for the reason you think.

Depending on how list is defined and initialized, push_back might throw an exception. If it does, the pointer returned from new is lost, and can never be freed.

But assuming push_back returns successfully, it stores a copy of the pointer returned by new, and so we can free the memory later by calling delete on that copy, so no memory is leaked as long as you do call delete properly.

jalf
  • 243,077
  • 51
  • 345
  • 550
  • I am sorry but i couldn't figure out whther the pointer is just copied or the pointed to data is also copied? For Ex :- String *str = new String("HI"); list.push_back(str); Now can i call delete str and still be able to access "HI" from the list because valgrind shows a leak at list.push_back without any delete calls. –  Jul 11 '14 at 07:04
6

No, the vector stores pointers and the copy is made of the pointer. You can delete the object any time later.

(You may get a leak, if the statement happens to throw an exception and you don't catch and handle it properly. That's why you might consider using smart pointers.)

UncleBens
  • 40,819
  • 6
  • 57
  • 90
  • 3
    "if the statement happens to throw an exception and you don't catch and handle it properly" - in the case of this statement there is no way to handle it properly, really. If `push_back` throws, then it hasn't stored the pointer, and there's no way to free it because this calling code doesn't have the pointer either. I suppose in theory you could maybe do something with `String::operator new`, but that doesn't sound like fun. – Steve Jessop Nov 24 '10 at 20:28
4

If I saw this code I would be very suspicious a memory leak was possible. On the surface it appears to be adding an allocated String* into a list<String*>. In my experience this is often followed by bad error handling code which does not properly free the allocated memory.

While this dangerous in many circumstances it is not necessarily a memory leak. Consider the following example:

class Container {
  ~Container() {
    std::list<String*>::iterator it = list.begin();
    while (it != list.end()) {
      delete *it;
      it++;
    }
  }

  void SomeMethod() {
    ...
    list.push_back(new String("hi"));
  }

  std::list<String*> list;
}

In this code there is no leak because the containing class is responsible for the memory allocated and will free it in the destructor.

EDIT

As aschepler pointed out there is still a leak if the push_back method throws an exception.

JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
1

You are correct, provided nothing deletes the string when it is removed from the list.

Matt K
  • 13,370
  • 2
  • 32
  • 51
1

Yes, this is a memory leak unles you somehow take steps to delete the contained pointers.

And the best way to accomplish that is to use a smart pointer. For example, Boost's shared_ptr or C++0x's shared_ptr.

John Dibling
  • 99,718
  • 31
  • 186
  • 324
0

No.

You can delete the object by doing:

delete list[i];
list.erase(list.begin() + i);

or clear the whole list by:

for (unsigned int i = 0; i < list.size(); ++i)
{
  delete list[i];
}
list.clear();
Patrizio Bertoni
  • 2,582
  • 31
  • 43
Snowfish
  • 7,479
  • 4
  • 20
  • 20
0
list.push_back(new String("hi"));

Why are you allocating dynamic strings in the first place? Unless you want to communicate between different parts of your program by changing strings (which would be quite unusual), get rid of the pointer:

std::list<std::string> list;         // note: no pointer!
list.push_back(std::string("hi"));   // explicitly create temporary
list.push_back("hi");                // alternative: rely on coercion
fredoverflow
  • 256,549
  • 94
  • 388
  • 662