Where to properly delete dynamically allocated object in loop in C++

Question

Tutorials, searches, and the dim memory of my C++ formal education have left me clueless as to where I should use delete when I'm using a dynamically allocated object pointer in a loop, such as:

// necessary files are included, this code is within main
T * t;
t = foo.getNewT();
while (!t->isFinalT()) {

    // print t stuff

    delete t;         // is this where I should delete t?
    t = foo.getNewT();
}

delete t;

This lack of knowledge has become particularly troublesome on a recent class project. On my laptop (Linux Mint, g++ Ubuntu/Linaro 4.7.3-1ubuntu1) the code ran fine without the delete statement and crashed when I added the delete statement. On the school server (Solaris, g++ (GCC) 3.4.5), the code segfaulted after a few iterations without the delete statement, and runs fine when I add the delete statement.

How do I handle this kind of loop properly so that it will run in most environments?

Additional Info: The error on my laptop occurs when the program reaches the delete request:

*** Error in 'program': free(): invalid next size (fast):...

Some of the other code:

// T.h
class T {
    int id;
    int num;
    int strVarPos;
    char * strVar;
  public:
    T();
    ~T();
    // + misc. methods
}

// T.cpp
T::T() {
    id = 0;
    num = -1;
    strVarPos = 0;
    char * strVar = new char[11];
    strVar[0] = '\0'
}

T::~T() {
    delete [] strVar;
}

// Foo.cpp
T * Foo::getNewT() {
    T * t = new T;

    // populate T's fields

    return t;
}

Resolution:

Because a simple test with just T * t and the loop worked ok, I ended up reconstructing the project starting from blank and adding one class at a time, to see when the problem would appear. Turns out that I had added additional content into a dynamically allocated array elsewhere in the program without updating the size constant I was using to initialize the array.

Evidently the school server could only handle the resulting memory discrepancy without crashing if I was making sure to delete the pointers properly (the program didn't run long enough to cause a significant memory leak in my tests), while my laptop wouldn't notice the memory discrepancy until I attempted to call delete (and then would crash).

If you want help identifying what is actually causing the crash, I suggest you post code for `getNewT()`, `T()`, and `~T()`, and maybe show the class declaration for `T`. — paddy, Oct 30 '13 at 21:56

Zac Howland · Accepted Answer · 2013-10-31T13:09:46.680

Assuming that foo.getNewT() is handing ownership of the memory over to the caller:

T * t;
t = foo.getNewT();
//while (!t->isFinalT()) // if foo.getNewT ever returns NULL, this will be UB!!!
while (t != nullptr && !t->isFinalT()) 
{
    // ...
    delete t; // if you now own it and are no longer going to use it, yes, delete it here
    t = foo.getNewT();
}
delete t; // you also need this one to delete the "final" t

However, you can avoid having to do it yourself by using std::unique_ptr:

std::unique_ptr<T> t;
t.reset(foo.getNewT());
while (t && !t->isFinalT()) 
{
    // ...
    t.reset(foo.getNewT());
}

Alternatively, you could rewrite the loop to flow a bit better:

std::unique_ptr<T> t;
do
{
    t.reset(foo.getNewT());
    if (t)
    {
        // do stuff with t
    }
} while (t && !t->isFinalT());

the code ran fine without the delete statement and crashed when I added the delete statement.

Are you sure getNewT is handing ownership of the T* to you? If you delete it, and then it tries to delete it later, you will end up with a heap corruption. If it is handing ownership over to the caller, and you do not delete it, you get a memory leak.

With the additional information in your edit:

char * strVar = new char[11];

That line is unnecessary if you declare strVar as either a std::string or a char[11]. If you attempt to copy any of those T objects, you'll be using the default copy constructor (as you have not defined one), which will do a shallow copy (that is, copy the value of the pointer for strVar). When you delete 2 Ts that are both pointing to the same memory location, you get a heap corruption. The most robust solution would be to declare strVar as a std::string.

You've mentioned some good general concerns; I don't believe they'll be of issue on this project, but I'll keep them in mind. (foo does not retain any references to the new T, and T.strVar will not be referenced out of the context of its T. Some of my pointer usage, such as strVar, is because I'm more comfortable with C than with C++) — Tydaeus, Nov 08 '13 at 05:56
The "final `t`" could also be `nullptr`. Wouldn't `delete`-ing it be UB, too? — Niklas R, Nov 08 '13 at 06:35
Deleting a null pointer does nothing. It is perfectly acceptable to have `T* p = nullptr; delete p;`. http://stackoverflow.com/questions/4190703/is-it-safe-to-delete-a-null-pointer — Zac Howland, Nov 08 '13 at 14:50

score 1 · Answer 2 · answered Oct 30 '13 at 21:44

1

The problem is not the delete. You have put it in the right place. It's more likely something else you are doing that is causing undefined behaviour.

Note that you should have a delete t after the loop as well (to catch the last one). This is assuming that foo.getNewT() always returns a valid pointer (which it must, because you never check if it is NULL).

answered Oct 30 '13 at 21:44

paddy

60,864
6
61
103

+1 (and I have suspicions the last assumption isn't going to make it, which would explain the OP's inconsistent results in platform-switching behavior). – WhozCraig Oct 30 '13 at 21:45

score 1 · Answer 3 · answered Oct 30 '13 at 21:45

You should delete a dynamically allocated memory when you no longer need it. If you want t to hold its value inside the for loop, then delete it outside the loop otherwise delete it inside.

However, the best thing to do is to use std::unique_ptr when you really have to use pointers . It will take care of deallocating the memory itself when all references to the memory are destroyed. You should try to avoid allocating memory as much as you can. Use STL containers if they fit the job.

score 0 · Answer 4 · answered Oct 30 '13 at 21:49

0

I think when you delete t you are deleting the real object inside your structure.

Maybe that what is causing the problem.

answered Oct 30 '13 at 21:49

hasan

23,815
10
63
101

Where to properly delete dynamically allocated object in loop in C++

4 Answers4