How to correctly overload + operator

Question

I am trying to define a simple class to work with 2d matrices, called Matrix2D01, and having trouble with the + operator. I have the += operator which is working fine,

Matrix2D01& Matrix2D01::operator+=(Matrix2D01& mat2) {
    int i,j;
    for (i=0;i<M;i++)
        for (j=0;j<N;j++) mat[i][j]+=mat2[i][j];
return *this;
}

The + operator is defined:

Matrix2D01 Matrix2D01::operator+(Matrix2D01& mat2) {
    Matrix2D01 result(1,1); 
    result=*this;
    result+=mat2;
    return result;
}

When i try to use the + operator, for example by

mat3=mat1+mat2;

the compiler gives an error:

../MatrixGames.cpp:17:12: error: no match for ‘operator=’ in ‘mat3 = Matrix2D01::operator+(Matrix2D01&)((* & mat2))’

If anyone can tell me what I'm doing wrong it will be greatly appreciated.

Thanks.

I also have a = operator defined,

Matrix2D01& Matrix2D01::operator=(Matrix2D01& mat2) {
    if (this==&mat2) return *this;
    int i,j;
    for (i=0;i<M;i++) {
        delete [] mat[i];
    }
    delete [] mat;
    M=mat2.Dimensions()[0];
    N=mat2.Dimensions()[1];
    mat = new double* [M];
    for (i=0;i<M;i++) {
        mat[i]=new double [N];
    }
    for (i=0;i<M;i++)
        for (j=0;j<M;j++)
            mat[i][j]=mat2[i][j];
    return *this;
}  

here is a complete test case:

#include <iostream>
#include <stdlib.h>
#include <stdio.h>
#include <vector>
using namespace std;
class Matrix2D01 {
protected:
    int D;
    double **mat;
    int M,N;
public:
    Matrix2D01(int m,int n);
    Matrix2D01(int m,int n,double d);
    ~Matrix2D01();
    vector <int> Dimensions() {vector <int> d(D,M); d[1]=N; return d;}
    double* operator[](int i) {return mat[i];}
    Matrix2D01& operator=(Matrix2D01& mat2);
    Matrix2D01& operator+=(Matrix2D01& mat2);
    Matrix2D01 operator+(Matrix2D01& mat2);
};
Matrix2D01::Matrix2D01(int m, int n) {
    int i,j;
    D=2;
    M=m;
    N=n;
    mat = new double* [M];
    for (i=0;i<M;i++) {
        mat[i]=new double [N];
    }
    for (i=0;i<M;i++)
        for (j=0;j<M;j++)
            mat[i][j]=0;
}

Matrix2D01::Matrix2D01(int m, int n,double d) {
    int i,j;
    D=2;
    M=m;
N=n;
mat = new double* [M];
for (i=0;i<M;i++) {
    mat[i]=new double [N];
}

for (i=0;i<M;i++)
    for (j=0;j<M;j++)
        mat[i][j]=d;
}

Matrix2D01::~Matrix2D01() {
int i;
    for (i=0;i<M;i++) {
        delete [] mat[i];
    }
    delete [] mat;
}
    Matrix2D01& Matrix2D01::operator=(Matrix2D01& mat2) {
    if (this==&mat2) return *this;
    int i,j;
    for (i=0;i<M;i++) {
        delete [] mat[i];
    }
    delete [] mat;
    M=mat2.Dimensions()[0];
    N=mat2.Dimensions()[1];
    mat = new double* [M];
    for (i=0;i<M;i++) {
        mat[i]=new double [N];
    }
    for (i=0;i<M;i++)
        for (j=0;j<M;j++)
            mat[i][j]=mat2[i][j];
    return *this;
}    
    Matrix2D01& Matrix2D01::operator+=(Matrix2D01& mat2) {
    int i,j,M2,N2;
    M2=mat2.Dimensions()[0];
    N2=mat2.Dimensions()[1];
    if ((M!=M2)||(N!=N2)) {
        cout<<"error: attempted to add non-matching matrices";
        return *this;
    }
    for (i=0;i<M;i++)
        for (j=0;j<N;j++) mat[i][j]+=mat2[i][j];
    return *this;
}    
    Matrix2D01 Matrix2D01::operator+(Matrix2D01& mat2) {
    Matrix2D01 result(1,1);
    result=*this;
    result+=mat2;
    return result;
}    
    int main() {
    Matrix2D01 mat1(2,2,1);
    Matrix2D01 mat2(2,2,2);
    Matrix2D01 mat3(2,2,4);
    mat3+=mat1;
    mat3=mat1+mat2;
    return 1;
}

Answer 1

Herb Sutter advocates the following canonical way to overload operator + (please read GotW, it is really well written and interesting):

• Don't include operator + as a member function, instead make it a free function.
• Pass one object by value and the other by const reference. This makes it possible to use move operators when you add to a temporary.

• Implement in terms of operator +=:

  Matrix2D01 operator+(Matrix2D01 a, const Matrix2D01 &b)
  {
      a += b;
      return a;
  }
  

Answer 2

I've posted the code of the Matrix class I had written for an assignment on Discrete Mathematics last Sem

Notice how the assignment operator and the copy constructor are implemented All you need to do is write an assignment operator (I'd recommend that you write a copy constructor as well)

#pragma once

#include <vector>
#include <istream>
#include <ostream>

class Matrix
{
private:
    unsigned int rows, cols;
    std::vector<std::vector<int> > matrix;

public:
    // Creates a Matrix with the given dimensions and sets the default value of all the elements to 0
    Matrix(unsigned int rows, unsigned int cols);
    // Destructor
    ~Matrix();

    // Converts the relation set into it's adjacency Matrix representation
    static Matrix CreateFromRelationSet( std::vector<std::pair<int, int> > relationSet);

    // Copy Constructor
    Matrix(const Matrix& cSource);
    // Assignment Operator
    Matrix& operator=(const Matrix& cSource);

    // Resets all the elements of the matrix to 0
    void Reset();
    // Resizes the matrix to the given size
    void Resize(unsigned int rows, unsigned int cols);

    // Returns the number of rows
    unsigned int getRows();
    // Returns the number of columns
    unsigned int getCols();

    // Returns the smallest element from the matrix
    int getSmallestElement();
    // Returns the greatest element from the matrix
    int getGreatestElement();

    // Returns true if element is found and sets the row and column
    // Returns false if not found
    bool getPosition(int element, int* i, int* j);

    // Deletes a row from the Matrix
    void DeleteRow(unsigned int row);
    // Deletes a column from the Matrix
    void DeleteColumn(unsigned int col);

    // Returns the element at (i,j)
    int& operator()(unsigned int i, unsigned j);
    // Returns the element at (i,j). Use the () operators instead
    int getElementAt(unsigned int i, unsigned j);

    friend std::ostream& operator << (std::ostream& out, Matrix& m);
    friend std::istream& operator >> (std::istream& in, Matrix& m);

    Matrix operator + (Matrix M);
    Matrix operator - (Matrix M);
    Matrix operator * (Matrix M);
};

// For use with cin & cout
std::ostream& operator << (std::ostream& out, Matrix& m);
std::istream& operator >> (std::istream& in, Matrix& m);


#include "Matrix.h"

Matrix::Matrix(unsigned int rows, unsigned int cols)
{
    this->rows = rows;
    this->cols = cols;

    matrix.resize(rows);
    for(std::vector<int>& i: matrix)
        i.resize(cols);

    Reset();
}

Matrix::~Matrix()
{

}

// local helper function
int findMaxFromSet(std::vector<std::pair<int, int> > set)
{
    int maxVal = 0;
    for(unsigned int i = 0; i < set.size(); i ++)
    {
        int p = set[i].first > set[i].second ? set[i].first : set[i].second;
        maxVal = maxVal > p ? maxVal : p;
    }

    return maxVal;
}

Matrix Matrix::CreateFromRelationSet (std::vector<std::pair<int, int> > relationSet)
{
    int max = findMaxFromSet(relationSet);
    Matrix M(max,max);
    M.Reset();

    for(auto i = relationSet.begin(); i != relationSet.end(); ++ i)
        M(i->first - 1, i->second - 1) = 1;

    return M;
}

void Matrix::Reset()
{
    for (auto& i: matrix)
        for(auto& j: i)
            j = 0;
}

void Matrix::Resize(unsigned int rows, unsigned int cols)
{
    matrix.resize(rows);
    for(auto& i:matrix)
        i.resize(cols);
}

unsigned int Matrix::getRows()
{
    return rows;
}

unsigned int Matrix::getCols()
{
    return cols;
}

Matrix::Matrix(const Matrix& cSource)
{
    rows = cSource.rows;
    cols = cSource.cols;
    matrix = cSource.matrix;
}

bool Matrix::getPosition(int element, int* i, int* j)
{
    for(unsigned int ii = 0; ii < getRows(); ii ++)
        for(unsigned int jj = 0; jj < getCols(); jj ++)
            if(matrix[ii][jj] == element)
            {
                *i = ii;
                *j = jj;
                return true;
            }

    return false;
}

Matrix& Matrix::operator=(const Matrix& cSource)
{
    // check for self-assignment
    if (this == &cSource)
        return *this;

    if(this->getRows() < cSource.rows)
    {
        this->rows = cSource.rows;
        this->matrix.resize(cSource.rows);
    }

    if(this->getCols() < cSource.cols)
    {
        this->cols = cSource.cols;
        for(auto& i:this->matrix)
            i.resize(cSource.cols);
    }

    for(unsigned int i = 0; i < rows; i++)
        for(unsigned int j = 0; j < cols; j++)
            this->matrix[i][j] = const_cast<Matrix&>(cSource)(i,j);

    return *this;
}

std::ostream& operator << (std::ostream& out, Matrix& m)
{
    for(auto& i: m.matrix)
    {
        for(auto& j: i)
            out<<j<<'\t';
        out<<std::endl;
    }
    return out;
}

std::istream& operator >> (std::istream& in, Matrix& m)
{
    for(auto& i: m.matrix)
        for(auto& j: i)
            in>>j;
    return in;
}

Matrix Matrix::operator + (Matrix op)
{
    // Find the rows and cols of the new matrix
    unsigned int r = this->getRows() > op.getRows()?this->getRows():op.getRows();
    unsigned int c = this->getCols() > op.getCols()?this->getCols():op.getCols();

    // Create Matrices
    Matrix A = *this;
    Matrix B = op;
    Matrix R(r,c);

    // Assign values
    for(unsigned int i = 0; i < A.rows; i++)
        for(unsigned int j = 0; j < A.cols; j++)
            R(i,j) = A(i,j) + B(i,j);

    return R;
}

Matrix Matrix::operator - (Matrix op)
{
    // Find the rows and cols of the new matrix
    unsigned int r = this->getRows() > op.getRows()?this->getRows():op.getRows();
    unsigned int c = this->getCols() > op.getCols()?this->getCols():op.getCols();

    // Create Matrices
    Matrix A = *this;
    Matrix B = op;
    Matrix R(r,c);

    // Assign values
    for(unsigned int i = 0; i < A.rows; i++)
        for(unsigned int j = 0; j < A.cols; j++)
            R(i,j) = A(i,j) - B(i,j);

    return R;
}

Matrix Matrix::operator* (Matrix op)
{
    Matrix A = *this;
    Matrix B = op;
    if(A.getCols() != B.getRows())
        throw std::exception("Matrices cannot be multiplied");

    Matrix M(A.getRows(), B.getCols());

    for(unsigned int i=0 ; i<A.getRows() ; i++)
        for(unsigned int j=0 ; j<B.getCols() ; j++)
            for(unsigned int k=0 ; k<B.getRows() ; k++)
                M(i,j) = M(i,j) + A(i,k) * B(k,j);

    return M;
}

int& Matrix::operator()(unsigned int i, unsigned j)
{
    return matrix[i][j];
}

int Matrix::getElementAt(unsigned int i, unsigned j)
{
    return (*this)(i,j);
}

int Matrix::getSmallestElement()
{
    int result = matrix[0][0];
    for(auto i:matrix)
        for(auto j : i)
            if(j < result)
                result = j;
    return result;
}

int Matrix::getGreatestElement()
{
    int result = matrix[0][0];
    for(auto i:matrix)
        for(auto j : i)
            if(j > result)
                result = j;
    return result;
}

void Matrix::DeleteRow(unsigned int row)
{
    matrix.erase(matrix.begin() + row);
    rows --;
}

void Matrix::DeleteColumn(unsigned int col)
{
    for(auto& i: matrix)
        i.erase(i.begin() + col);
    cols --;
}

UPDATE:

    result=*this;

In your code you are trying to allocate result the value of *this using the assignment operator, but no assignment operator is defined and the compiler doesn't find a match
Writing an assignment operator overload will solve this issue.

And if you have a copy constructor you can do something like:

Matrix2D01 Matrix2D01::operator+(Matrix2D01& mat2) {
    Matrix2D01 result = *this;
    result+=mat2;
    return result;
}

Writing Assignment Operators: http://www.learncpp.com/cpp-tutorial/912-shallow-vs-deep-copying/
And if you're interested in the details - here you go: http://www.icu-project.org/docs/papers/cpp_report/the_anatomy_of_the_assignment_operator.html

Answer 3

Your operands should be const references. Otherwise, you can't use a temporary to initialize them. (In this case, the return value of operator+ is a temporary, and thus, cannot be bound to the non-const reference of operator=.)

If your operator+ is a member, it should be const as well. After all, it doesn't modify the object it is called on. So:

Matrix2D01& Matrix2D01::operator=( Matrix2D01 const& mat2 );
Matrix2D01& Matrix2D01::operator+=( Matrix2D01 const& mat2 );
Matrix2D01 Matrix2D01::operator+( Matrix2D01 const& mat2 ) const;

Also, your assignment operator is broken. (Think of what will happen, if one of the allocations fails after you've freed the original data.) In general, if you have to test for self assignment, the assignment operator is broken.