Am I using delete correctly here?

Question

I've just started combining my knowledge of C++ classes and dynamic arrays. I was given the advice that "any time I use the new operator" I should delete. I also know how destructors work, so I think this code is correct:

main.cpp

...
int main()
{
    PicLib *lib = new PicLib;
    beginStorage(lib);
    return 0;
}

void beginStorage(PicLib *lib)
{
...
    if (command != 'q')
    {
        //let's assume I add a whole bunch
            //of stuff to PicLib and have some fun here
        beginStorage(lib);
    }
    else
    {
        delete lib;
        lib = NULL;
        cout << "Ciao" << endl;
    }
}

PicLib.cpp

...

PicLib::PicLib()
{
    database = new Pic[MAX_DATABASE];
    num_pics = 0;
}

PicLib::~PicLib()
{
    delete[] database;
    database = NULL;
    num_pics = 0;
}
...

I fill my PicLib with a Pic class, containing more dynamic arrays. Pic's destructor deletes them in the same manner seen above. I think that delete [] database gets rid of all those classes properly.

So is the delete in main.cpp necessary? Everything looking hunky dory here?

Answer 1

There are a couple of problems:

int main() 
{ 
  PicLib *lib = new PicLib; 
  beginStorage(lib); 
  return 0; 
}

It is best to allocate and delete memory in the same scope so that it is easy to spot.

But in this case just declare it locally (and pass by reference):

int main() 
{ 
    PicLib  lib; 
    beginStorage(lib); 
    return 0; 
}

In beginStorage()

But I see no reason to manipulate a pointer. Pass it by reference and just use it locally.

void beginStorage(PicLib& lib)
{
 ....
}

In the PicLib class you have a RAW pointer: databases.

If you have a RAW pointer that you own (you create and destroy it) then you must override the compiler generated versions of the copy constructor and assignment operator. But in this case I see no reason touse a pointer it would be easier to just use a vector:

class PivLib
{
    private:
        std::vector<Pic>   databases;
};

Answer 2

Yes, anything you create with new must be deleted with delete, and anything created with new[] must be deleted with delete[], at some point.

There are some things I'd point out in your code though:

• Prefer std::vector<> over using new[] and delete[]. It'll do the memory management for you. Also have a look at smart pointers like auto_ptr, shared_ptr and in C++0x unique_ptr for automatic memory management. These help save you from forgetting a delete and causing a memory leak. If you can, don't use new/new[]/delete/delete[] at all!
• If you have to new and delete something, it's a very good idea to new in a class constructor, and delete in a class destructor. When exceptions are thrown, objects go out of scope etc., their destructors are called automatically, so this helps prevent memory leaks. It's called RAII.
• Having beginStorage delete its parameter is potentially a bad idea. It could crash if you call the function with a pointer not created with new, because you can't delete any pointer. It'd be better if main() created a PicLib on the stack rather than using new and delete, and for beginStorage to take a reference and not delete anything.

Answer 3

Yes it is necessary, unless you use an auto_ptr (and read up on the semantics of auto_ptr before you use it -- you can't copy it arround).

for example :

int main()
{
    auto_ptr<PicLib> lib = new PicLib;
    beginStorage(lib);
    return 0;
} // auto_ptr goes out of scope and cleans up for you

Answer 4

else doesn't go with while. You'd want something more like:

void beginStorage(PicLib *lib)  
{  
    while (command != 'q') 
    { 
        //let's assume I add a whole bunch 
            //of stuff to PicLib and have some fun here 
    } 

    delete lib; 
    lib = NULL;  // Setting to NULL is not necessary in this case,
                 // you're changing a local variable that is about
                 // to go out of scope.
    cout << "Ciao" << endl;
}

The delete looks good, but you should make sure to document that beginStorage takes ownership of the PicLib object. That way anyone using beginStorage knows that they don't have to delete it later.

Answer 5

The delete in main.cpp is necessary.

This is probably a matter of personal preference, but I would advise against calling new and delete in separate logical parts (here the delete call on the PicLib instance is in a separate function). Usually it's better to have the responsibility for allocation and deallocation given to just one part.

@AshleysBrain has a better suggestion (about creating PicLib the stack), although this might cause problems if PicLib takes up too much memory.

Answer 6

Generally you want to delete in the same place as you new. It makes the accounting easier. Better is to use a smart pointer (scoped_ptr in this case), which means that your code is still correct even if the body of the while throws an exception and terminates prematurely.

Answer 7

Everything looks good.

The delete in main.cpp is necessary because if you didn't call delete then the destructor is not run and your array is not deleted. You'd also be leaking the memory for the class, not just the array.

Answer 8

Yes, otherwise the destructor for PicLib will not be called.

One stylistic note though: If you new a method-scope object in a function, try and delete it in the same function. As a junior engineer that has had to go through large projects fixing other people's memory leaks...this makes things a lot easier for other people to read. From the looks of it, you could delete *lib after beginStorage() returns. Then it would be easier to see the scope of *lib in one place.