Destructor and Classes in C++ [ Memory Leak]

Question

I have a problem freeing up memory. First I go to show my constructors , destructors and private part of the clases:

Class 1:

class VectorDinamico {

private:
    int util, tam;
    int *vect;

   /*Rest of the class*/
}

VectorDinamico::VectorDinamico() {
    util = 0;
    tam = 10;
    vect = new int[10];
}

VectorDinamico::VectorDinamico(const VectorDinamico &v){
    vect = new int[v.tam];

    for (int i = 0 ; i < v.util; i++) {
        vect[i] = v.vect[i];
    }
    util = v.util;
    tam = v.tam;
 }

VectorDinamico::~VectorDinamico() {
    delete []vect;
}

VectorDinamico& VectorDinamico::operator= (const VectorDinamico &v) {
    if (this != &v) {
        delete []vect;

        vect = new int[v.tam];

        for (int i = 0 ; i < v.util; i++) {
            vect[i] = v.vect[i];
        }
        util = v.util;
        tam = v.tam;
    }
    return *this;
}

Works fine ( Valgrind: nothing in use at exit)

Class 2:

class Conjunto {
private:
    VectorDinamico datos;

    /*Rest of the class*/
}

//Is empty because it uses the VectorDinamico constructor( I think )
Conjunto::Conjunto() {}

Conjunto::Conjunto( const Conjunto &c) {
   datos = c.datos; // = Is overloaded in VectorDinamico
}

//Is empty because it uses the VectorDinamico destructor( I think )
Conjunto::~Conjunto() {};

Conjunto& Conjunto::operator=( const Conjunto &c) {
    if (this != &c)
        datos = c.datos;

    return *this;
}

Works fine ( Valgrind: nothing in use at exit)

Class 3: ( I think here is the problem )

class SetConjuntos{
private:
    Conjunto *conjuntos;
    int util, tam;

    /*Rest of the class*/
}

SetConjuntos::SetConjuntos(){
    util = 0;
    tam = 10;
    conjuntos = new Conjunto[10];
}

SetConjuntos::SetConjuntos(const SetConjuntos &sc){
    util = sc.util;
    tam = sc.tam;
    conjuntos = new Conjunto[tam];

    for(int i = 0 ; i < util ; i++)
    conjuntos[i]=sc.conjuntos[i];
}
//Here is my problem ( I think )
//anyway I've left it empty, because I think it uses the Conjunto destructor's
SetConjuntos::~SetConjuntos(){
    //delete []conjuntos; ( Cause segmetation fault )
}

I think the SetConjuntos destructor is not correct because the Valgrind output is:

==6838== LEAK SUMMARY:
==6838==    definitely lost: 2,912 bytes in 4 blocks
==6838==    indirectly lost: 11,320 bytes in 180 blocks

How must be the destructor for SetConjuntos ?

Thanks

-----------Solved adding operator= to SetConjuntos----------------

I had not implemented the assignment operator, because he thought that if he did not use explicitly was not necessary. I was wrong.

Now class 3 is:

class SetConjuntos{
private:
    Conjunto *conjuntos;
    int util, tam;

    /*Rest of the class*/
}

SetConjuntos::SetConjuntos(){
    util = 0;
    tam = 10;
    conjuntos = new Conjunto[10];
}

SetConjuntos::SetConjuntos(const SetConjuntos &sc){
    util = sc.util;
    tam = sc.tam;
    conjuntos = new Conjunto[tam];

    for(int i = 0 ; i < util ; i++)
    conjuntos[i]=sc.conjuntos[i];
}

SetConjuntos::~SetConjuntos(){
    delete []conjuntos; //(  NO cause segmetation fault )
}

SetConjuntos& SetConjuntos::operator=(const SetConjuntos &sc){
    if(this != &sc){
        util = sc.util;
        tam = sc.tam;
        Conjunto *conjuntos_loc = new Conjunto[tam];

        for(int i = 0 ; i < util ; i++)
            conjuntos_loc[i]=sc.conjuntos[i];

        delete[] conjuntos;
        conjuntos = conjuntos_loc;
    }
    return *this;
}

Answer 1

The code shown has no evident errors, so the mistake is in something you didn't place in the question.

As a wild guess you're (possibly involuntarily) using copy construction or assignments and this creates problems with the ownership.

Remember the rule: either you don't have a destructor, an assignment operator or a copy constructor or probably you will need all three of them.

This rules is so important that if you happen to meet a case in which you don't need a copy constructor but you need a destructor (can't think to one, but it could exist) please document it clearly with a comment that it's not something you forgot about.

Answer 2

I just want to point out to you that there is better ways to write a copy assign operator. Let's take a look at your code:

VectorDinamico& VectorDinamico::operator= (const VectorDinamico &v) {
    if (this != &v) {
        delete []vect;
            /* What happens here is that you delete vect [ ], but what
               happens if the next line 'vect = next int [ v.tam ];' throws
               an exception?
            */
        vect = new int[v.tam];

        for (int i = 0 ; i < v.util; i++) {
            vect[i] = v.vect[i];
        }
        util = v.util;
        tam = v.tam;
    }
    return *this;
}

Take a look at The rule of three, here you will get detailed explanation how to write a copy assign operator.

Answer 3

You appear to be learning the C++ of 15 years ago.

1. use std::vector<int> instead of writing your own VectorDinamico
   • this already has correct copy, assignment, etc.

2. if you can't do 1, use std::unique_ptr<int[]> instead of the raw int *vect. It saves you having to write the destructor at all. You still need to write the copy and copy-assignment though.

   • the canonical correct way to write the copy assignment is first to write a swap method or overload, like so:

     void VectorDinamico::swap(VectorDinamico &other) {
         std::swap(util, other.util);
         std::swap(tam,  other.tam);
         std::swap(vect, other.vect);
     }
     

     and then write the copy assignment operator so it just reuses the copy constructor, swap and destructor:

     VectorDinamico& VectorDinamico::operator=(VectorDinamico const &other) {
         VectorDinamico tmp(other);
         tmp.swap(*this);
     }
     

     the benefit, apart from much simpler code with less repetition, is that if the allocation fails throwing bad_alloc, both original vectors are left in a well-defined state (your original code leaves the left-hand one broken)

3. use std::unique_ptr to wrap the raw pointer members of your other two classes as well, and stop writing the destructors manually

4. if you're writing the copy constructor and copy assignment operator, you should probably consider writing the move versions as well

5. if you're running valgrind and it finds a leak, it will tell you where that memory was allocated. Just make sure you have debugging symbols in your build

6. if you're not sure whether your destructors are being called, just add a print statement and see, or step through in the debugger. Seeing comments like I think this is called here just tells me you haven't tried finding out.

7. and finally to your immediate problem:

   //Here is my problem ( I think )
   //anyway I've left it empty, because I think it uses the Conjunto destructor's
   SetConjuntos::~SetConjuntos(){
       //delete []conjuntos; ( Cause segmetation fault )
   }
   

   your conjuntos is a raw pointer - you've deleted them manually everywhere else, what's different here? You should un-comment this delete.

   if you get a segmentation fault with this un-commented, run it under gdb or get a core dump and see where. Also see if valgrind or glibc warn you about double freeing.

   Almost certainly, one of the array members you're deleting is damaged, and finding the origin of that damage is your real problem.

Answer 4

The destructor of SetConjutos is fine as you had it:

SetConjuntos::~SetConjuntos(){
    delete [] conjuntos;
}

However if you have pointer members (in VectorDinamico and SetConjutos ) you Need to define a copy constructor and assignment Operator:

VectorDinamico::VectorDinamico(const VectorDinamico &v) :
    util(v.util),
    tam(v.tam),
    vect(new int[10])
{
    //copy Content of v.vect to vect
}

VectorDinamico& Operator=(const VectorDinamico &v)
{
    //Copy util, tam and the CONTENT of vec
    return *this;
}

Note: You Need to define a copy constructor and assignment Operator for SetConjuntos as well.

When you don't have a copy constructor and assignment Operator it is possible that you are trying to free the Memory multiple times.

And as you can see in my copy constructor, I am using an initializer list which I also recommend to use.