Two Dimensional Matrices in C++

Question

I'm trying to do some operations in two dimensional matrices. I overloaded (+ , - and *) to do the calculations. I have a problem regarding (I believe) memory management. Look at the following code:

Mtx M1(rows1,cols1,1.0); //call the constructor 
Mtx M2(rows2,cols2,2.0); //call the constructor 
Mtx M3(rows3,cols3,0.0); //call the constructor 

M3 = M1 + M2;
cout << M3 << endl;

Mtx Mtx::operator+(const Mtx &rhs)
{

double **ETS;
ETS = new double*[nrows];
for (int i = 0; i < rhs.nrows; i++) {
    ETS[i] = new double[rhs.ncols];
}
if (ETS == NULL) {
    cout << "Error Allocation on the Heap" << endl;
    exit(1);
}

for (int i = 0; i < rhs.nrows; i++) {
    for (int j = 0; j < rhs.ncols; j++) {
        ETS[i][j] = 0.0;
    }
}

for (int i = 0; i < rhs.nrows; i++) {
    for (int j = 0; j < rhs.ncols; j++) {
        ETS[i][j] = ets[i][j];
    }
}


for (int i = 0; i < rhs.nrows; i++) {
    for (int j = 0; j < rhs.ncols; j++) {
        ETS[i][j] = ETS[i][j] + rhs.ets[i][j];
    }
}

Mtx S(nrows, ncols, ETS);
delete [] ETS;
return S;
}

I think my problem is here:

Mtx S(nrows, ncols, ETS); 
delete [] ETS;
return S;

Is this a proper way to return ETS? Or do you think the problem is with the constructor? I got no output when I did the above return!

This is the constructor for Mtx S(nrows, ncols, ETS);

Mtx::Mtx(int rows, int cols, double **ETS)
{
ets = new double*[nrows];
for (int i = 0; i < nrows; i++) {
    ets[i] = new double[ncols];
}
for (int i = 0; i < nrows; i++) {
    for (int j = 0; j < ncols; j++) {
        ets[i][j] = ETS[i][j];
    }
  }
}

My copy constructor:

Mtx::Mtx(const Mtx& rhs)
:nrows(rhs.nrows), ncols(rhs.ncols)
    {
ets = new double*[nrows];
for (int i = 0; i < nrows; i++) {
    ets[i] = new double[ncols];
}

for (int i = 0; i < rhs.nrows; i++) {
    for (int j = 0; j < rhs.ncols; j++) {
        ets[i][j] = rhs.ets[i][j];
    }
  }
}

I overloaded << to print M3. It works fine because I tested printing M1 and M2.

I also did the following, and still not working:

Mtx S(nrows, ncols, ETS);
for (int i = 0; i < rhs.nrows; i++) {
    delete [] ETS[i];
}
delete [] ETS;
return S;
}

What was wrong with [`boost/numeric/ublas/matrix.hpp`](http://www.boost.org/doc/libs/release/libs/numeric/ublas/doc/matrix.htm) that meant you had to write your own? — johnsyweb, Jul 22 '12 at 03:12
@Johnsyweb: In the link, you can replace version with "release" to always point to latest release, i.e. http://www.boost.org/doc/libs/release/libs/numeric/ublas/doc/matrix.htm — , Jul 22 '12 at 03:16
@Johnsyweb I didn't know about [link](http://www.boost.org/doc/libs/1_42_0/libs/numeric/ublas/doc/matrix.htm). I have to write my own. — Jack in the Box, Jul 22 '12 at 03:17
@JackintheBox: Why do you have to write your own? Also, what does your copy constructor look like? You shouldn't need to use `new`/`delete` here at all. — johnsyweb, Jul 22 '12 at 03:18
The problem may be in `class Mtx`'s copy constructor because `return S` requires a copy construction. If you didn't do deep copy, the result would be wrong. — timrau, Jul 22 '12 at 03:21
@Johnsyweb because I wanted to learn how to create my own. I'm practicing :) — Jack in the Box, Jul 22 '12 at 03:22
[Programmers often confuse multidimensional arrays with arrays of pointers](http://stackoverflow.com/questions/4810664/how-do-i-use-arrays-in-c). You seem to have done this. — johnsyweb, Jul 22 '12 at 03:31
Your copy constructor leaks the memory that was previously allocated to the object. Since your class manually manages a resource (memory) you should study [The Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). In addition, you should learn good practices for [Operator Overloading](http://stackoverflow.com/questions/4421706/operator-overloading). — Blastfurnace, Jul 22 '12 at 04:16
@Blastfurnace I tested the following: `M3 = M1` and it worked fine. So, do you think the problem is in the copy constructor? — Jack in the Box, Jul 22 '12 at 04:35
`M3 = M1` uses the assignment operator. You haven't posted `Mtx::operator=` but I'm guessing it also leaks memory. Have you spent some time reading the link in my previous comment about [The Rule Of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three)? — Blastfurnace, Jul 22 '12 at 04:43
@Blastfurnace I also outputted the result of `M3 = M1 + M2` inside `operator+` and it worked! — Jack in the Box, Jul 22 '12 at 04:43
@Blastfurnace Yeah, I think you are right about the assignment operator. I read about The Rule Of Three before, and this not my first time working with this. Why are you guessing that the copy constructor leaks memory? What did you notice? — Jack in the Box, Jul 22 '12 at 04:46
On second look, your copy constructor looks okay. Sorry about the confusion. What is the specific problem again? The `operator+` returns a blank matrix? — Blastfurnace, Jul 22 '12 at 04:51
@JackintheBox: Your `operator+()` returns an `Mtx` instance, not "nothing". — johnsyweb, Jul 22 '12 at 11:43
@Johnsyweb I agree it returns `Mtx`, but I don't see it! `cout << M3 << endl;` doesn't print the output which means there is a problem in the return. — Jack in the Box, Jul 22 '12 at 21:20
Your code says `Mtx M1(rows1,cols1,1.0); //call the constructor` but you've not provided us with a ctor that takes a `double` as the third and final argument. Please provide an [SSCCE](http://sscce.org/). — johnsyweb, Jul 23 '12 at 12:24

score 3 · Answer 1 · 2012-07-22T05:19:40.313

3

You could instead use

std::vector< std::vector<double> >

instead of

double **

that will be more safe as far as bounds are concerned. And you only have to use functions

std::vector::size()

and

std::vector::clear()

in the destructor. :)

edited Jul 22 '12 at 05:19

answered Jul 22 '12 at 05:07

score 2 · Answer 2 · answered Jul 22 '12 at 03:43

Yes, there is a problem where you pointed out. You have to delete all the memory that is pointed to in the pointers in ETS. So, it should be more like:

for (int i = 0; i < rhs.nrows; i++) {
    delete [] ETS[i];
}
delete [] ETS;

I good rule is that for each call to new you have to call delete; You allocate the array with nrows+1 calls to new so you have to delete with the same number. That's not hard and fast but it'll clue you in when something is going wrong.

score 2 · Answer 3 · edited Jul 22 '12 at 21:52

The common way to implement binary arithmetic operators is to define operator+= as a class member function and operator+ as a free-standing function. Note the + operator is implemented in terms of the += operator and it takes its left operand by copy. As long as your copy constructor and assignment operator are correct this should work.

// member function
Mtx& Mtx::operator+=(const Mtx &rhs)
{
    for (int i = 0; i < nrows; ++i) {
        for (int j = 0; j < ncols; ++i) {
            ets[i][j] += rhs.ets[i][j];
        }
    }
    return *this;
}

// non-member function
Mtx operator+(Mtx lhs, const Mtx &rhs)
{
    lhs += rhs;
    return lhs;
}

Two Dimensional Matrices in C++

3 Answers3