Where are the memory leaks?

Question

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.

Well the code above is not used so it can't leak. Show us how you use it (and the class definition) and then we can explain how it leaks. Don't forget the rule of three. — Martin York, May 14 '11 at 18:51
Your code violates the [rule of three](http://stackoverflow.com/questions/4172722/). You'll get a lot more memory leaks and undefined behavior once you start copying lemons. — fredoverflow, May 14 '11 at 19:05
How many leaks does it have without the second constructor (`Lemon(int)`)? How many does it have with `lines` but no `matrix`? — Beta, May 14 '11 at 19:35

score 4 · Accepted Answer · answered May 14 '11 at 18:38

4

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.

answered May 14 '11 at 18:38

I would possibly, but I'm just trying to get more familiar with multidimensional arrays. – ogward May 14 '11 at 18:47
@ogward You can use vector to implement multi-dimensional arrays. – May 14 '11 at 18:55
thanks for the tip, I need to get more familiar with vectors. Everyone keeps telling me to use them. – ogward May 14 '11 at 18:57

score 1 · Answer 2 · answered May 14 '11 at 18:37

1

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.

answered May 14 '11 at 18:37

Marius Bancila

16,053
9
49
91

If you find yourself needing to use `this->` to remove ambiguity you can probably rename some of the variables instead and get a better way to remove the ambiguity. – Martin York May 14 '11 at 18:52
I agree with that. I don't think I use this except for passing a pointer to the current object to some function. – Marius Bancila May 15 '11 at 08:52

score 1 · Answer 3 · answered May 14 '11 at 18:38

1

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

answered May 14 '11 at 18:38

Pontus Gagge

17,166
1
38
51

I have a readFromfile function and when the first number of the textfile is read i initiate nrOf to that number. – ogward May 14 '11 at 18:46
That may be so, but you allocate memory without recording how much you've allocated. Are you engaging in some sort of gamble as to whether the first number in the file happens to match your allocation? You seem to be losing... :-) – Pontus Gagge May 14 '11 at 18:47
ok I've manged to remove all my memory leaks by moving my read from file function into my constructor, but that is really ugly...Is there any other way I can do it? I want to read the first line of the file then create my two arrays and then send the info from the files into the arrays. – ogward May 14 '11 at 18:55

score 1 · Answer 4 · answered May 14 '11 at 19:37

1

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;
}

answered May 14 '11 at 19:37

Roddy

66,617
42
165
277

the problem is that the size of the matrix is read from a text file. – ogward May 14 '11 at 21:09
@ogward: and so...? You'll still need to construct one of these. – Roddy May 14 '11 at 22:15

score 0 · Answer 5 · answered May 14 '11 at 18:37

0

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.

answered May 14 '11 at 18:37

Ben Jackson

90,079
9
98
150

Serge Dundich · Answer 6 · 2011-05-14T23:39:36.890

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;
    }
}

Where are the memory leaks?

6 Answers6