views:

295

answers:

4

I'm using Visual Studio 2008, Developing an OpenGL window. I've created several classes for creating a skeleton, one for joints, one for skin, one for a Body(which is a holder for several joints and skin) and one for reading a skel/skin file.

Within each of my classes, I'm using pointers for most of my data, most of which are declared using = new int[XX]. I have a destructor for each Class that deletes the pointers, using delete[XX].

Within my GLUT display function I have it declaring a body, opening the files and drawing them, then deleting the body at the end of the display. But there's still a memory leak somewhere in the program. As Time goes on, it's memory usage just keep increasing, at a consistent rate, which I'm interpreting as something that's not getting deleted.

I'm not sure if it's something in the glut display function that's just not deleting the Body class, or something else. I've followed the steps for memory leak detection in Visual Studio 2008 and it doesn't report any leak, but I'm not 100% sure if it's working right for me. I'm not fluent in C++, so there maybe something I'm overlooking, can anyone see it?

From main:

void display(void){
    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
    Body *body = new Body();
    body->readSkel("C:\\skel2.skel");
    body->drawBody();
    body = new Body();
    body->readSkel("C:\\skel1.skel");
    body->drawBody();
    glutSwapBuffers();  
    body->~Body();
    delete body;
}

From Body:

Body::Body(){
    skelFile = string();
    skinFile = string();
    totalJoints = 0;
    joints = new Joint[25];
    skin = new Skin;
}

Body::~Body(){
    delete[25] joints;
    delete skin; 
}
+1  A: 

It would help if you'd paste in a little code, but I would:

Double check your syntax: int *foo = new int[size]; delete[] foo;

Ensure all child classes who's parents use dynamic memory also contain destructors, even if the destructor is an empty statement.

Asklepius M.D.
Code is listed below. I've made sure that for every new, I have a delete.
Nicholas
maybe I haven't had enough coffee this morning, but I see two new Body() statements allocating memory, but only one call to the destructor. What happens to your first Body?
Asklepius M.D.
+5  A: 

In this code:

Body *body = new Body();
body->readSkel("C:\\skel2.skel");
body->drawBody();
body = new Body();

you're leaking a Body because you don't delete the first one.

And this:

body->~Body();
delete body;

is just weird. You don't explicitly call destructors like that - the delete takes care of calling the destructor.

This code:

delete[25] joints;

is also weird. The correct form is:

delete [] joints;

You're using a non-standard syntax, and the 25 will be ignored. See this question for more information.

RichieHindle
My bad, I was swapping them in my code and actually had one commented out... and that works, I see what I did there... Thank you very much.
Nicholas
The extra destructor call for body will probably result in a double delete of joints and skin - I'm surprised this didn't crash.
Mark B
@RichieHindle - `body->~Body(); delete body` is not just weird but could cause undefined behavior in his example. The destructor will get called twice and since his code is not setting `joints` or `skin` to 0, the code will double delete which can cause UB.
R Samuel Klatchko
As I said, in my actual code, there both aren't actually there, just one or the other, I was just doing some debugging.
Nicholas
A: 

Jochen Kalmbach is your friend.

Vulcan Eager
+3  A: 

Real programmers can write FortranJava in any language! Java requires that you allocate (practically) everything dynamically, but C++ does not.

Since nobody else has pointed it out (at least directly), in display there seems to be no reason to use dynamic allocation at all. Just do something like:

void display(void){
    glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
    Body body;
    body.readSkel("C:\\skel2.skel");
    body.drawBody();

    Body body2;
    body2.readSkel("C:\\skel1.skel");
    body2.drawBody();
    glutSwapBuffers();  
}

If your readSkel clears the existing skeleton data, you don't need to define body2, but without knowing that, this is the easy way to keep things safe.

Likewise, in your definition of Body, you don't seem to be doing anything that requires dynamic allocation either.

class Body { 
    std::string skelFile;
    std::string skinFile;
    int totalJoints;
    Skin skin;
    Joint joints[25];
public:
    Body() : totalJoints(0) {}
};

or better still:

class Body { 
    std::string skelFile;
    std::string skinFile;
    Skin skin;
    std::vector<Joint> joints;
public:
   // presumably other stuff goes here...but you don't need a ctor or dtor.
};

This gets rid of most chances for leaking anything (at least in these parts of the code -- since we haven't seen your Skin or Joint classes, it's hard to guess what they may be doing...

Jerry Coffin
Yup, great advice. Don't use dynamic allocation unless you have to.
jalf