3

I am attempting to create an overloaded operator for a matrix class that I have built. My matrix class stores the matrix in a dynamically allocated multidimensional array. I am simply trying to test my overloaded operator by multiplying two matrices together that are exactly the same and display the output. I am getting weird results and I believe it has to do with one of the conditionals on my for loops. I have however walked through all of my for loops and can find nothing wrong. The matrices I am multiplying together are both 6x6.

My overloaded operator

template <typename T>
const matrix<T> matrix<T>::operator * (const matrix& right) const
{
    matrix<T> c = right;
    int sum_elems;
    for( int i = 0; i < this->rows - 1; ++i)
    {
        for(int j = 0; j < right.cols - 1; ++j)
        {
            sum_elems = 0;
            for( int k = 0; k < right.rows - 1; ++k)
            {
                sum_elems += this->the_matrix[i][k] * right.the_matrix[k][j];
            }

            c.the_matrix[i][j] = sum_elems;
        }
    }
    return c;
}                 

Now my call to the overloaded operator in my main function:

std::cout << my_matrix;
matrix<int> copy_matrix;
copy_matrix = my_matrix * my_matrix; 
std::cout << copy_matrix;

My output:

  The Matrix:
 0  1  0  1  1  0
 1  0  1  0  1  1
 0  1  0  1  0  1
 1  0  1  0  1  0
 1  1  0  1  0  1
 0  1  1  0  1  0
   The Matrix:
 -1  33  139587680  18  38  75
 139587680  18  38  75  157  1
 139587712  38  1470  4365  10411  1
 139587744  75  4365  19058932  64514866  0
 139587776  157  10411  64514866  1136204102  1
 139596144  1  1  0  1  0

As you can see it seems that I am stepping out of bounds on one of my arrays. I can not seem to find where though. I appreciate your help in advance.

Edit: As requested my full implementation of my matrix class

Matrix Definitions:

template <typename T>
class matrix
{
    public:

        //Default Constructor
        matrix();

        //Overloaded Constructor
        matrix(std::ifstream&, const char*);

        //Copy Constructor
        matrix(const matrix&);

        //Destructor
        ~matrix();


        //overloaded operators
        T* operator [] (T);
        const matrix operator * (const matrix&) const;
        matrix& operator = (const matrix&);
        friend std::ostream& operator << <T> (std::ostream&, const matrix<T>&); 


    private:
        T** the_matrix;
        unsigned rows, cols;

Matrix Implementation:

/* Template version of matrix class */
/*---------------------------------------------------------------------------*/
// Default contructor
template <typename T>
matrix<T>::matrix() { }

// Overloaded contructor
template <typename T>
matrix<T>::matrix( std::ifstream& in, const char* file)
{

    // declare the variables to be used
    T vertices, edges, u, v;
    std::string line;

    // open file for reading
    in.open(file);

    // get number of vertices
    in >> vertices;


    // throw away second line   
    std::getline(in, line);
    std::getline(in, line);

    // get number of edges and dump them in two arrays
    in >> edges;
    T edge1 [edges];
    T edge2 [edges];
    int j = 0, k = 0;
    for(int a = 0; a < edges; ++a)
    {    
        in >> u >> v;
        edge1[j] = u;
        edge2[k] = v;
        ++j;
        ++k;
    }

    in.close();

    // Create multi-dim-dynamic array
    rows = vertices;
    cols = vertices;

    the_matrix = new T*[rows];

    for( int b = 0; b < rows; ++b)
    {
        the_matrix[b] = new T [rows];
    }

    // Initialize array values to zero
    for ( int c = 0; c < rows; ++c)
    {
       for( int d = 0; d < cols; ++d)
       {
           the_matrix[c][d] = 0;
       }
    }

    // push the edges to the matrix
    for( int e = 0; e < edges; ++e)
    {
        the_matrix[edge1[e] - 1][edge2[e] - 1] = 1;
    }
    for ( int f = 0; f < edges; ++f)
    {
        the_matrix[edge2[f] - 1][edge1[f]-1] = 1;
    }


}

// Copy Constructor
template <typename T>
matrix<T>::matrix(const matrix& left)
{
    the_matrix = left.the_matrix;
    rows = left.rows;
    cols = left.cols;
    spath = left.spath;
}

// Destructor
template <typename T>
matrix<T>::~matrix()
{ 
    // Deletes the data in reverse order of allocation
    for( int a = cols; a > 0; --a)
    {
        delete[ ] the_matrix[a];
    }

    delete[ ] the_matrix;
}

// Overloaded * Operator
template <typename T>
const matrix<T> matrix<T>::operator * (const matrix& right) const
{
    matrix<T> c = right;
    T sum_elems;
    for( int i = 0; i < this->rows - 1; ++i)
    {
        for(int j = 0; j < right.cols - 1; ++j)
        {
            sum_elems = 0;
            for( int k = 0; k < right.rows - 1; ++k)
            {
                sum_elems += this->the_matrix[i][k] * right.the_matrix[k][j];
            }

            c.the_matrix[i][j] = sum_elems;
        }
    }
    return c;
}

// Overloaded assignment Operator
template <typename T>
matrix<T>& matrix<T>::operator = (const matrix& right)
{
    this->the_matrix= right.the_matrix;
    this->rows = right.rows;
    this->cols = right.cols;
    this->spath = right.spath;
    return *this;
}

// Overloaded << operator
template <typename T>
std::ostream& operator << (std::ostream& output, const matrix<T>& left)
{
    // Test screen output to see if correct   
    std::cout << std::setw(14) << "The Matrix:" << '\n';
    for( int a = 0; a < left.rows; ++a)
    {
        for( int b = 0; b < left.cols; ++b)
        {
            std::cout << ' ' << left.the_matrix[a][b] << ' ';
        }
        std::cout << '\n';
    }
    return output;
}
  • Does your matrix class copy itself correctly? Both in the copy constructor and the assignment operator? You say you use a "dynamically allocated multidimensional array". Does that mean you are manually managing your memory with `new` and `delete`, rather than using a vector? If so, why? – Benjamin Lindley Feb 19 '12 at 06:54
  • Copy constructor and assignment operator work flawlessly. I am not using a `vector` specifically because I am trying to learn more about using operator `new` and `delete` and manually managing memory instead of always relying on `vector` to do the behind the scenes work. –  Feb 19 '12 at 06:57

2 Answers2

5

As I suspected, your copy constructor and assignment operator are in fact not implemented correctly. You are simply copying the pointer over. That means that when you copy one matrix to another, they both share the same data. When one of them goes out of scope, the destructor is called, then the shared data is deleted, leaving the remaining matrix with dangling pointers.

Fix those functions so they actually allocate new arrays, and copy the data.

Benjamin Lindley
  • 101,917
  • 9
  • 204
  • 274
  • You are absolutely correct. I didn't realize I was just making a copy of the pointers. Once I fixed the copy and and assignment operator my * operator is working perfect. Thanks for your help! –  Feb 19 '12 at 17:27
1
  1. i actually goes from 0 to this->rows - 2 (because of i < n-1 for i = n-1 is false). Same for other loops. This seems not to be correct behaviour for matrices multiplication.
  2. Nevertheless, this code fragment seems to be correct. Can you provide us with full class implementation?

P.S. If T is type of matrix elements, then type of sum_elems should be T.

Artem Sobolev
  • 5,891
  • 1
  • 22
  • 40