Sum of matrices using dynamic allocation and operator overloading

Question

I have written a code to find sum of two matrices using dynamic allocation and operator overloading.

#include<iostream>
#include<new>
using namespace std;
class matrix
{
    int**m;
    int r;
    int c;
public:
    matrix(int a, int b)
    {

        r = a;
        c = b;
        m = new int*[r];
        for (int i = 0; i < r; i++)
            m[i] = new int[c];
    }
    ~matrix()
    {
        for (int i = 0; i < r; i++)
            delete[] m[i];
        delete[] m;
    }
    friend istream &operator>>(istream &in, matrix s);
    friend matrix operator+(matrix& m1, matrix& m2);
    friend ostream &operator<<(ostream &out, matrix s);

};
istream &operator>>(istream &in, matrix s)
{

    cout << "enter elements" << endl;
    for (int i = 0; i < s.r; i++)
    {
        for (int j = 0; j < s.c; j++)
        {
            in >> s.m[i][j];
        }
    }
    return in;
}
ostream &operator<<(ostream &out, matrix s)
{

    for (int i = 0; i < s.r; i++)
    {

        cout << " ";
        for (int j = 0; j < s.c; j++)
            out << s.m[i][j] << " ";
        cout << endl;
    }
    return out;
}
matrix operator+(matrix& m4, matrix& m5)
{
    matrix m6(m4.r, m4.c);
    for (int i = 0; i < m4.r; i++)
    {
        for (int j = 0; j < m4.c; j++)
        {
            m6.m[i][j] = m4.m[i][j] + m5.m[i][j]; //gets stuck here!
        }

    }
    return m6;
}
int main()
{
    int r, c;
    cout << "enter number of rows and columns" << endl;
    cin >> r >> c;
    matrix m1(r, c), m2(r, c), m3(r, c);
    cin >> m1;
    cin >> m2;
    m3 = m1 + m2;
    cout << m3;
    return 0;
}

When I execute, I get stuck at matrix operator+(matrix &m4,matrix &m5).

I searched on internet but I'm unable to find my error.So, what's wrong with my code? It works fine in Code::Blocks but not in Xcode.

Answer 1

Problems I see in your code:

1. You have code to allocate memory in constructor and deallocate memory in the destructor. However, you have not implemented a copy constructor or a copy assignment operator. This leads to lots of problems. See The Rule of Three.

2. The second argument to operator>> needs to be a reference. Otherwise, you'll end up reading data into a local variable. There won't be any change to the object that you used to call it.

   Another unwanted side effect of not having implemented the copy constructor is that you end up using dangling pointers when this function returns, which is cause for undefined behavior.

3. The second argument to operator<< should be a const&. Otherwise, you'll needlessly make a copy of the object and delete it.

   This function will also result in dangling pointers due to the missing copy constructor.

The copy constructor can be implemented as:

matrix(matrix const& copy) : r(copy.r), c(copy.c)
{
   m = new int*[r];
   for (int i = 0; i < r; i++)
   {
      m[i] = new int[c];
      for (int j = 0; j < c; ++j )
      {
         m[i][j] = copy.m[i][j];
      }
   }
}

I'll leave you to implement the copy assignment operator.

Answer 2

The problem stems from pass by value in place of pass by reference, but this exposed a complete lack of Rule of Three compliance.

R Sahu has demonstrated the copy constructor to resolve the Rule of Three problem.

I'm going to take a different route: Die Array, DIE!

Unlike a standard stupid array, std::vector is Rule of Three Compliant. Using it for 2 Dimensions is pretty painless:

std::vector<std::vector<int>> mMatrix;

Vector makes this program rule of 3/5 compliant, which means you can copy and move Matrix objects with dramatically reduced debugging adventures. You gain nothing from using an array instead of a vector unless the array is of a fixed size, so think carefully before using one.

Note the change in name. matrix is already used by the class, so a bit of fuddle takes care of naming confusion. I do a bit more renaming for similar reasons:

size_t mRows; // can't have negative indexes, so why used a signed type?
size_t  mCols;

The constructor now looks like this:

matrix(size_t rows, size_t cols) :
            mRows(rows), mCols(cols), mMatrix(rows, std::vector<int>(cols))
{
}

The friend functions get a slight make-over to get better naming and to use references where they are needed:

friend std::istream &operator>>(std::istream &in, matrix &out);
friend matrix operator+(matrix& rhs, matrix& lhs);
friend std::ostream &operator<<(std::ostream &out, matrix &in);

And the rest can go pretty much unchanged.

But there is an even better way to do this:

#include<iostream>
#include<vector>
//using namespace std;
class matrix
{
    size_t mRows; // can't have negative indexes, so why used a signed type?
    size_t  mCols;
    std::vector<int> mMatrix; 
    //  the vector is one dimensional, so tyhere is much less clean-up
    // In addition, all of the data is contiguous and as cache-friendly as possible.
public:
    matrix(size_t rows, size_t cols) :
            mRows(rows), mCols(cols), mMatrix(rows * cols)
    {
    }

    int & operator()(size_t row, size_t col)
    {
        if (row >= mRows || col >= mCols)
        { // trap bad indexing. discard if speed > safety.
            throw std::out_of_range("Bad index");
        }
        return mMatrix[row * mCols + col]; // note the math
    }

    // returns a copy so the value can't be modified
    int operator()(size_t row, size_t col) const
    {
        if (row >= mRows || col >= mCols)
        { // trap bad indexing. discard if speed > safety.
            throw std::out_of_range("Bad index");
        }
        return mMatrix[row * mCols + col]; // note the math
    }

    friend std::istream &operator>>(std::istream &in, matrix &out);
    friend matrix operator+(matrix& rhs, matrix& lhs);
    friend std::ostream &operator<<(std::ostream &out, matrix &in);

};

This layout is much faster because of spatial locality assisting in caching data as you're working. The 2 D array approach has number of rows +1 different array that may be in very different places in RAM, leading to an increase in cache misses and time lost to reading data that could have already been read if all the data was in one place.