CODEWITHSUNDEEP
c++memory-leaksmultidimensional-array
ogward
ogward

Reputation: 1193

Where are the memory leaks?

I have this piece of code and for some reason it creates 6 memory leaks and I can't figure out where.

    Lemon::Lemon()
{
    this->nrOf=0;
    int x=10;
    this->matrix=new int*[x];
    this->lines=new string[x];
    for (int i = 0; i < x; ++i) 
        this->matrix[i] = new int[x];
    for(int i=0;i<x;++i)
        for(int j=0;j<x;++j)
            this->matrix[i][j]=-1;

}
Lemon::Lemon(int n)
{
    this->x=this->nrOf;
    this->matrix=new int*[x];
    this->lines=new string[x];
    for (int i = 0; i < x; ++i) 
        this->matrix[i] = new int[x];
    for(int i=0;i<x;++i)
        for(int j=0;j<x;++j)
            this->matrix[i][j]=-1;
}

Lemon::~Lemon()
{
    for(int i=0;i<this->nrOf;i++)
    {
        delete []this->matrix[i];
    }
    delete []this->matrix;
    delete []this->lines;
}

Any kind of help is appreciated.

Upvotes: 1

Views: 168

Answers (6)

Serge Dundich
Serge Dundich

Reputation: 4429

Note 1:

Every new-expression in your code is possible memory leak because your code is awfully exception unsafe: if any of your new expressions throw an exception all your already new-allocated storage is leaked.

Note 2:

Your first constructor does not semantically match your destructor (even if we assume that everything goes fine without throwing): constructor assigns zero value to this->nrOf but allocates 10 elements of this->matrix array and assign allocated array to each element BUT destructor expects this->nrOf elements of this->matrix (arrays) to delete so 0 elements is deleted and thus 10 elements are leaked.

Side note: your this->x (that exists according to second constructor) is not initialized by first constructor.

Note 3:

Your second constructor is simply corrupt: you never initialize this->nrOf variable but you use it like it contains something valid.

ADD:

Good advise here would be: never use arrays and new/new[]/delete/delete[] expressions. Use std::vector in your situation instead. This way you may never care about memory leaks, exception safety, etc.

Your matrix would look like this:

std::vector< std::vector<int> > matrix;
....................
....................
n = 10;
m = 20;
matrix.resize(n);

for( size_t j = 0; j < n; ++j )
{
    matrix[j].resize(m);

    for( size_t k = 0; k < m; ++k )
    {
        matrix[j][k] = j+k;
    }
}

Upvotes: 0

Roddy
Roddy

Reputation: 68033

Try this as your only constructor

Lemon::Lemon(int n = 10)  // default constructor
{
  nrOf = n;

  matrix = new int*[nrOf];
  lines = new string[nrOf];

  for (int i = 0; i < nrOf; ++i) 
    matrix[i] = new int[nrOf];

  for(int i=0; i<nrOf; ++i)
    for(int j=0; j<nrOf; ++j)
        this->matrix[i][j] = -1;
}

Upvotes: 1

Pontus Gagge
Pontus Gagge

Reputation: 17258

You don't set nrOf to the number of allocated matrix rows in either constructor.

Upvotes: 1

user2100815
user2100815

Reputation:

At this point:

this->x=this->nrOf;

nrOf has not been initialised - you have undefined behavior.

And possibly you would benefit from using std::vector.

Upvotes: 4

Ben Jackson
Ben Jackson

Reputation: 93710

You seem to have both a member variable int x (used in Lemon::Lemon(int n)) as well as a local variable int x in Lemon::Lemon(). Also, you do not consistently set nrOf in your constructors.

Upvotes: 0

Marius Bancila
Marius Bancila

Reputation: 16318

The default constructor initializes nrOf with 0, but you allocate a matrix of 10x10. Which means the destructor will not run the for loop, since nrOf is 0.

BTW, you don't have to prefix everything with this. It kinda makes the code harder to read. You should use this only to eliminate ambiguity.

Upvotes: 1

Related Questions