Appropriate destruction of certain class members

Question

I have a class with several pointer members that can be reallocated. When I use the LoadOBJ() function, I'm supposed to replace the already held data but I'm having trouble with garbage collection. Below is some code.

class Object3d{

public:

int nVertex;
int nFace;
int nVertexNormal;

Vector3d *vertex;
Vector3d *vertexNormal;
Face3d *face;

void LoadOBJ(char*);
Object3d():nVertex(0), nFace(0), vertex(NULL), face(NULL){}
Object3d(char*);
~Object3d(){}
};

Face3d:

struct Face3d{
int nFaceV;
int *iVertex;
int *iVertexNormal;

Face3d():nFaceV(0){}
};

Everytime I load a new object with the LoadOBJ() function, I want to delete the previously allocated memory, rather than just use new and leak previously allocated memory.

I'm not sure how to do this. This is what I thought of for now:

void *vGarbage, *vGarbage2, 
     *fGarbage,
     *iGarbage, *iGarbage2;

//nFace is the previous number of faces; in the case of a new object, it's 0
            for(int i=0; i<nFace; i++) 
            {
                    iGarbage=face[i].iVertex;
                    iGarbage2=face[i].iVertexNormal;
                    delete[] iGarbage;
                    delete[] iGarbage2;
            }

            vGarbage=vertex; 
            fGarbage=face; 
            vGarbage2=vertexNormal;

            delete[] vGarbage;
            delete[] vGarbage2;
            delete[] fGarbage;

The above code runs everytime I use LoadOBJ(), but there still is memory leak. I'm also wondering if this is the right way to do it?

To clarify where the problem/question is: why do I still have memory leak? And, is there better/cleaner way to delete the previously allocated memory?

Answer 1

Check out C++11's smart_pointers, they provide the ability of allocating memory which, when the object goes out of scope, will be freed automatically.

#include <memory>
#include <iostream>

struct Foo {
    Foo() { std::cout << "Foo...\n"; }
    ~Foo() { std::cout << "~Foo...\n"; }
};

struct D { 
    void operator()(Foo* p) const {
        std::cout << "Call delete for Foo object...\n";
        delete p;
    }
};

int main()
{
    {
        std::cout << "constructor with no managed object\n";
        std::shared_ptr<Foo> sh1;
    }

    {
        std::cout << "constructor with object\n";
        std::shared_ptr<Foo> sh2(new Foo);
        std::shared_ptr<Foo> sh3(sh2);
        std::cout << sh2.use_count() << '\n';
        std::cout << sh3.use_count() << '\n';
    }

    {
        std::cout << "constructor with object and deleter\n";
        std::shared_ptr<Foo> sh4(new Foo, D());
    }
}

Output:

constructor with no managed object constructor with object Foo... 2 2 ~Foo... constructor with object and deleter Foo... Call delete for Foo object... ~Foo...

(http://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr)

Remember that for each new a delete should be called when freeing memory. Local pointers can be dangerous if they get destroyed and you didn't free memory before that point.

The RAII paradigm in object-oriented C++ has been designed specifically to make resource management (and also memory management) easy.

If disaster has already been done, you can clean your code up with something like http://deleaker.com/ or equivalent memory leak-seeker software.

Also: if you can't use C++11 or you can't use a C++11-supporting compiler, take a chance of implementing smart pointers yourself, it shouldn't be too hard and will surely help your memory problems.

Answer 2

I understand you want to free the memory occupied by Object3d::vertex, Object3d::vertexNormal and Object3d::face before reasigning these members. First, you should provide a custom destructor for your Face3d so that you no longer need to care for it's members in the containing class. That is:

face3d::~face3d() {
    if (iVertex) delete[] iVertex;
    if (iVertexNormal) delete[] iVertexNormal;
}

In your Object3d class, you can use a dedicated clean-up function:

void Object3d::cleanup() {
    if (face) delete[] face;
    face = nullptr;
    if (vertex) delete[] vertex;
    vertex = nullptr;
    if (vertexNormal) delete[] vertexNormal;
    vertexNormal = nullptr;
    nVertex = 0;
    nFace = 0;
    nVertexNormal = 0;
}

Btw, In the destructor Object3d::~Object3d() you must call that function as well.

Answer 3

This question might answer yours. I think that you have to cast the void pointer to a specific one, like int*, to make it work. But the behaviour is highly dependent of the compiler you use.

edit: the advice of using smart pointers is probably the easiest and safest way of solving your problem.

Answer 4

Use std::vector instead of manually managed arrays:

struct Face3d{
int nFaceV;
std::vector<int> iVertex;
std::vector<int> iVertexNormal;

Face3d():nFaceV(0){}
};

class Object3d{

public:

std::vector<Vector3d> vertex;
std::vector<Vector3d> vertexNormal;
std::vector<Face3d> face;

void LoadOBJ(char*);
Object3d():nVertex(0), nFace(0), vertex(NULL), face(NULL){}
Object3d(char*);
~Object3d(){}
};

This frees you from the burden to write destructors. As already said above, this is exemplifies the RAII pattern in C++ which should be used instead of manual resource management.

As a general comment, public data members are almost always a bad idea because it breaks encapsulation. Object3d should provide some services to clients and keep its internal private.