Adding two matrices prints a column of garbage data c++

Question

I'm trying to add two matrices using multidimensional arrays and overloading the addition operator '+' and the assignment operator '=' however, garbage data is passed to my overloaded assignment operator function.

I've been reading things about the Copy and Swap Idioms and so on but I can't seem to find a solution to my problem, and I'd prefer to use + and = separately, rather than '+=' which I've seen some people do, because I will use other arithmetic for matrices, but I want to get this solved first.

Here is my header:

#ifndef Matrix_h
#define Matrix_h
#include <stdio.h>
#include <stdlib.h>
#include <iostream>
#include <utility>

class Matrix
{
private:
    int row;
    int column;
    double  ** elements;

public:
    Matrix();
    Matrix(int r, int c); //constructor
    Matrix(const Matrix& src);  //copy constructor
    ~Matrix(); //destructor

    Matrix& operator=(const Matrix& right);  // assignment operator
    int getRow() const;
    int getColumn() const;
    void setElement(int r, int c, double data);
    double getElement(int r, int c) const;
    Matrix& operator+(const Matrix& right); // calculates the sum of two matrices

};

#endif 

Here are my function definitions:

#include <string.h>
#include "matrix.h"
using namespace std;

Matrix::Matrix(int r, int c){ //defines the constructor to create a new matrix
    row = r;
    column = c;
    elements = new double*[row];
    for (int i=0; i<row; i++){
        elements[i] = new double[column];
    }
    for (int i=0; i<row; i++){
        for (int j=0; j<column; j++){
            elements[i][j] = 0;
        }
    }
}

Matrix::Matrix(const Matrix& src){ //defines the copying constructor
    row = src.row;
    column = src.column;
    elements = new double*[row];
    for (int i=0; i<row; i++){
        elements[i] = new double[column];
    }
    for (int i=0; i<row; i++){
        for (int j=0; j<column; j++){
            elements[i][j] = src.elements[i][j];
        }
    }
}

Matrix::~Matrix() { //defines the destructor
    for (int i=0; i<row; i++){
        delete[] elements[i];
    }

    delete[] elements;
};

void Matrix::setElement(int r, int c, double data) {
    elements[r][c] = data;
};

double Matrix::getElement(int r, int c) const {
    return elements[r][c];
}

int Matrix::getColumn() const {
    return column;
}

int Matrix::getRow() const {
    return row;
}

Matrix& Matrix::operator =(const Matrix& right) {

    if(this->elements != right.elements && column==right.column && row==right.row)
    {
        memcpy ( &this->elements, &right.elements, sizeof(this->elements) );
     }

    return *this;
}

Matrix& Matrix::operator +(const Matrix& right) {
    // first, make sure matrices can be added. if not, return original matrix
    if (this->row != right.row || this->column != right.column){
        cout << "Matrix sizes do not match.";
        return (*this);
    }

    Matrix sum(row, column);
    for (int i=0; i<this->row; i++){
        for (int j=0; j<this->column; j++){
            sum.elements[i][j] = this->elements[i][j] + right.elements[i][j];

        }
    }
    return sum;
}

My main simply creates matrixA with a 4x4 matrix of 1s and matrixB with a 4x4 matrix of 2s and adds them where the sum = matrixC which should result in a 4x4 matrix of 3s. However, this is my result:

matrixA
1   1   1   1   
1   1   1   1   
1   1   1   1   
1   1   1   1   

matrixB
2   2   2   2   
2   2   2   2   
2   2   2   2   
2   2   2   2   

matrixC before addition
0   0   0   0   
0   0   0   0   
0   0   0   0   
0   0   0   0   

matrixC after addition
1.0147e-316 3   3   3   
1.0147e-316 3   3   3   
1.0147e-316 3   3   3   
1.0147e-316 3   3   3

When I test it to see what elements are passed to 'right' in the '=' function I noticed similar data as the first column of matrixC. I believe I'm not understanding the passing of references involved here fully so I'd like some help on that.

Answer 1

Hmm, my CLang compiler warns me that operator +(const Matrix& right) return a reference to a temporary which is Undefined Behaviour.

You must return a plain object and not a reference:

Matrix operator=(const Matrix& right);  // assignment operator

Answer 2

Matrix& Matrix::operator +(const Matrix& right) {
    // ...

    Matrix sum(row, column);
    // ...
    return sum;
}

You're returning a reference to an automatic local (sum).

You can see the canonical forms of the arithmetic operators here, and addition should look like

Matrix Matrix::operator+(const Matrix &b) const;

There are various other issues, some mentioned in other answers, but the only thing your current operator+ reliably produces is Undefined Behaviour.

Answer 3

Your assignment operator is broken (and logically flawed).

It's logically flawed because an assignment like m1 = m2 should result in m1 == m2, no matter what the previous state of m1 was. Your implementation only attempts to copy when they have the same size. You should resize the matrix on the left side of the assignment.

Your operator also isn't using memcpy correctly. memcpy(dest, src, count) will copy contiguous data from the address src to the address dest. The addresses you're providing are the addresses of pointers to pointers to int, not pointers to a contiguous block of ints. There are two levels of indirection here that memcpy simply has no clue about. Also, sizeof(this->elements) is sizeof(int**), which will be a constant of probably 4 or 8 bytes, which should also seem wrong. To fix this, I would just use a plain old set of for loops.

Matrix& Matrix::operator=(const Matrix& other){
    // attempt to acquire memory before modifying (for exception safety)
    int** temp = new int*[other.row];
    for (int i = 0; i < other.row; ++i){
        temp[i] = new int[other.col];
    }

    // cleanup
    for (int i = 0; i < row; ++i){
        delete[] elements[i];
    }
    delete[] elements;

    // copy data
    elements = temp;
    row = other.row;
    col = other.col;
    for (int i = 0; i < row; ++i){
        for (int j = 0; j < col; ++j){
            elements[i][j] = other.elements[i][j];
        }
    }

    return *this
}

Another important note: Your operator+ returns a reference to a local variable. This is very bad, but C++ doesn't protect you from it. You should just return sum by value.

In modern C++, we generally prefer to use safer alternatives to manual memory management like std::vector<std::vector<int>>, which go a long way to seamlessly enforce correct usage and to save you from yourself.

Answer 4

A hint: if you store the matrix as one array double m[rows * columns] the implementation is going to be much simpler.

Indexing [row][column] is m[row * columns + column].

Summing is just summing two arrays element-wise (vector sum essentially).