How to free memory from a list of classes

Question

Say I have two classes created work and workItem.

        CWorker *work = new CWorker();
        CWorkItem *workItem = new CWorkItem();

The work class has a public list m_WorkList and I add the work item to it.

work->m_WorkList.push_back(workItem);

If I just delete work

if(work != NULL)
delete work;

Do I need to loop through the list in the destructor like the following? Any better way to do this? Could I use clear instead?

while(m_WorkList.size())
{
    CWorkItem *workItem = m_WorkList.front();
    m_WorkList.pop_front();
    if(workItem)
        delete workItem;
}

Answer 1

Yes you need to delete each item. If you call new N times, then you need to call delete exactly N times as well. There is no shortcut for bulk deleting items.

Also when you're done with it you need to call delete on work.

You can use new[] and delete[] if you want to create an array of items on the heap and release the items on the heap respectively at once. But in your case this isn't what you're looking for.

You can look to boost::shared_ptr if you don't want to manually do these delete calls.

Answer 2

Assuming that you don't maintain a list of workItems elsewhere, then yes, you'll need to delete each of them individually before you delete the Worker itself (thus losing the references). If the Worker holds the only list, then the deconstructor is as good a place as any (otherwise you'll need to delete each workItem "manually" before you call delete on the Worker itself)

However, if the workItems references exist elsewhere, you can choose the appropriate time to delete them, which may or may not be when you delete Worker.

Answer 3

as said in the other responses, you need to loop over your list and delete each elements. if you call clear, it will just remove pointers from the list not pointed objects.

if you need a list with the notion of ownership you can use boost pointer container. it will ensure that your objects are destroyed when you destroy your list

Answer 4

It really depends on the ownership, which is what you need to decide on when building the interface. Say you have a method:

CWorker::AddItem(CWorkItem *workItem)

You need to specify who owns the memory, and therefor who should delete it. Depending on what you are trying to do, it could make sense for the caller to own it (like if items should be routinely shared between CWorkers), or for the CWorker to own it (if each CWorkItem belongs to a single CWorker). Whoever owns it is responsible for deleting it.

In the former case, you could take the the WorkItems as shared_ptrs, to specify the ownership is shared between Workers, and you wouldn't need to do manual deletes. In the latter case, you could also use a specialized container like ptr_vector to obviate the need for manual deletes, but you should also but a note on the function that you are taking control of the memory.

Also, as a note, delete is null-safe, so you don't need those if statements, and it would be faster to pop_back() than pop_front()

Answer 5

use boost shared_ptr (which will soon become a standard)

  typedef boost::shared_ptr<WorkItem> WorkItemPtr;
  ....
  std::list<WorkItemPtr> list;
  ...
  list.push_back(new WorkItem);

Now when list is deleted the WorkItems will be deleted for you. You can pass WorkItemPtrs around in your code and not have to worry about them.

Answer 6

As others have said, you need a delete with every new. The container won't do this for you by default. You don't need to pop the items off the list though. You can create a functor to do the work for you. The following is common practice for people who can't use tr1::shared_ptr or Boost.

struct deleter
{
    template <class T>
    void operator()(const T* ptr) const
    { 
        delete ptr;
    }
};

// Usage
std::for_each(work->m_WorkList.begin(), work->m_WorkList.end(), deleter());
delete work;