Matrix overloading

Question

I'm making a 2D dynamic Matrix class. Problem arises in my copy constructor and =operator. Kindly tell me what am I doing wrong. Here is the code: (The cout's are for checking.

private: 
int rows;
int coloumns;
float **ptr;

Matrix(const Matrix & M)
{     cout << "copy const called"<<endl;

    cout << "1 "<< endl;
    if(rows < 0 || column < 0)  // To check if its a garbage value or not
    {
        rows = 0, col = 0;
        ptr = NULL;
        cout << "2 "<< endl;
    }

    else if(ptr!=NULL)
    {
        cout << "3 "<< endl;
        for(int i = 0 ; i < col; i++)
        {
            delete [] ptr[i];
        }
        cout << "4 "<< endl;
        delete [] ptr;
        ptr = NULL;
        cout << "5 "<< endl;
    }
    cout << "6 "<< endl;

    *this = M;
    cout << "7 "<< endl;
}           
Matrix operator= (const Matrix &M)
{
    if(this == &M)
    {
        return *this;
    }

    if(row!=0 && columns != 0)
    {
        for(int i = 0 ; i < columns; i++)
        {
            delete [] ptr[i];
        }
        delete [] ptr;
        ptr = NULL;
    }
    rows = M.rows; col = M.columns;

        ptr = new float *[rows];
        for(int i = 0; i < rows; i++)
        {
            ptr[i] = new float [col];
        }

        for(int i = 0; i< rows ; i++)
        {
            for( int j=0 ; j< columns ;j++)
            {
                ptr[i][j] = M.ptr[i][j];
            }
        }

    return *this;
}
 int main()
 {
   Matrix M1(2,3);
   Matrix M2(M1);
   M2(0, 0) = 1;
 }

It stops at the " *this = M " in the copy constructor. Moreover, I wanted to confirm that when I return something in the = operator, does it take the place of the whole " *this = M" , or just replaces M?

Note: Not allowed to use vectors.

Answer 1

Your code looks complicated to me.

I do not understand why you have chosen float** instead of plain float*. This looks better to me:

int rc, cc; // row count, column count
float* d; // data (rc * cc floats)

Memory allocation became a simpler operation:

d = new float[ rc * cc ];

Copying is also simpler:

memcpy( d, source.d, rc * cc * sizeof( *d ) );

The "hard" part is to retrieve a matrix element. You have to convert a row and column to an index:

index = row * column_count + column;

The whole class:

#include <iostream>

class matrix_type
{
  int rc, cc; // row count, column count
  float* d; // data

  void allocate( const int arc, const int acc ) // a prefix in arc and acc stands for Argument
  {
    if ( arc * acc == rc * cc )
      return; // nothing to do: already allocated
    delete[] d;
    rc = arc;
    cc = acc;
    d = new float[rc * cc];
  }

  void copy( const matrix_type& s )
  {
    allocate( s.rc, s.cc );
    memcpy( d, s.d, rc * cc * sizeof( *d ) );
  }

  int as_idx( const int ar, const int ac ) const
  {
    return ar * cc + ac;
  }

public:
  matrix_type( const int arc, const int acc ) : rc( 0 ), cc( 0 ), d( 0 )
  {
    allocate( arc, acc );
    memset( d, 0, rc * cc * sizeof( *d ) );
  }

  matrix_type()
  {
    delete[] d;
  }

  matrix_type( const matrix_type& s ) : rc( 0 ), cc( 0 ), d( 0 )
  {
    copy( s );
  }

  matrix_type& operator=(const matrix_type& s)
  {
    copy( s );
    return *this;
  }

  float& at( const int ar, const int ac )
  {
    if ( ar < rc && ac < cc )
      return d[as_idx( ar, ac )];
    else
      throw "out of range";
  }

  const float& at( const int ar, const int ac ) const
  {
    return const_cast<matrix_type*>(this)->at( ar, ac );
  }

  void print( std::ostream& os ) const
  {
    for ( int r = 0; r < rc; ++r )
    {
      for ( int c = 0; c < cc; ++c )
        os << at( r, c ) << ' ';
      os << '\n';
    }
  }

};

int main()
{
  matrix_type m1( 3, 5 );
  m1.at( 0, 0 ) = 1.f;
  m1.at( 2, 4 ) = 15.f;

  matrix_type m2( m1 );

  matrix_type m3( 0, 0 );
  m3 = m2;

  m3.print( std::cout );

  return 0;
}

Answer 2

You have infinite recursion going on. In you copy constructor you have

*this = M;

This calls your class's operator= which you have declared as

Matrix operator= (const Matrix &M)

You can see that you are returning by value. When you return by value a copy is made. To make that copy we need to call the copy construct. This in turn calls the assignment operator again which call the copy constructor and the cycle just keeps going.

Your copy constructor can be corrected and simplified to be

Matrix(const Matrix & m) : rows(m.rows), columns(m.columns)
{
    ptr = new float*[rows];
    for (int i = 0; i < rows; i++)
        ptr[i] = new float[columns];

    for (int i = 0; i < rows; i++)
        for (int j = 0; j < columns; j++)
            ptr[i][j] = m.ptr[i][j]
}

Notice how I didn't have to check the state of the new class as we know what it is since we are initializing it. Everything is uninitialized and all we have to do is initialize everything and then copy the values from the one matrix into the new one.

In regards to you assignment operator you should have it return a reference to the object instead of returning by value. This avoids unnecessary copies and allows you to chain the operator with other operators.