C++: is push_back(new Object()) a memory leak?

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

edi开发者_如何学JAVAt: 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

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.

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.)

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.

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

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.

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

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();