C++ operator overloading fails to output + operation

Viewed 196

I'm learning C++ and trying to write a C++ class for matrices, where I store the matrix as a one-dimensional C array. To this end, I defined an element member function to access the matrix elements based on their location in the array. I have then overloaded the << and + operators to handle the display and addition of matrices. The << operator works as intended, as can be demonstrated in the following example:

#include<iostream>

class matrix {
    friend std::ostream & operator<<(std::ostream &os, matrix &M);
private:
    int rows{}, columns{};
    double *array {nullptr};
public:
    matrix(int rows, int columns);
    ~matrix() {}

    double & element(int r, int c);

    matrix operator+(matrix &M)
    {
        matrix N(rows, columns);
        if (M.rows!=rows || M.columns!=columns){
            std::cout << "ERROR: DIMENSIONS OF MATRICES DO NOT MATCH" << std::endl;
        }
        else{
            for (int i{1}; i<=M.rows; i++)
            for (int j{1}; j<=M.columns; j++){
                N.element(i,j) = element(i,j) + M.element(i,j);
            }
        }
        return N;
    }
};

matrix::matrix(int r, int c)
{
    rows = r;
    columns = c;
    array = new double[rows*columns];
    for(int i{0}; i<rows*columns; i++) array[i]=0.0;
}

double & matrix::element(int i, int j)
{
    int loc{0};
    loc = (j-1) + (i-1)*columns;
    return array[loc];
}

std::ostream & operator<<(std::ostream &os, matrix &M)
{
    for (int i{1}; i<=M.rows; i++){
        os << "[ ";
        for (int j{1}; j<=M.columns; j++){
            os << M.element(i,j) << " "; 
        }
        os << "] \n";
    }
    return os;
}


int main() {
    matrix A(2,2);
    matrix B(2,2);
    A.element(1,1) = 1;
    A.element(1,2) = 2;
    A.element(2,1) = 3;
    A.element(2,2) = 4;
    B.element(1,1) = 1;
    B.element(1,2) = 2;
    B.element(2,1) = 3;
    B.element(2,2) = 4;
    std::cout << A << std::endl;
    std::cout << B << std::endl;

    return 0;
}

I'm then unable to display the addition of two matrices:

    std::cout << A+B << std::endl;

If I, however, break up the operation and add the matrices separately before displaying their sum, I get the right output, which makes me believe that the + operator also works correctly:

    matrix C(2,2);
    C = A+B;
    std::cout << C << std::endl;

The error seems to suggest that there may be an issue converting the matrix elements into the ostream, but it's curious that the above workaround solution works.

3 Answers

Your << operator takes a non-const reference to a matrix as parameter. That means it can't take reference to a temporary object. The result of A+B is a temporary object if you don't assign it to something.

So you need to change this:

std::ostream & operator<<(std::ostream &os, matrix &M);

into this:

std::ostream & operator<<(std::ostream &os, const matrix &M);

You may sooner or later encounter the same problem with your +-operator:

matrix operator+(matrix &M)

should be

// Both `M` and `*this` should be const
matrix operator+(const matrix &M) const

Naturally you will then have a problem with your element method which can only act on a non-const object, so you also need a variant that acts on a const object:

class matrix
{
   ....
 public:
   double & element(int r, int c);
   double element(int r, int c) const;
   ...
}

You can't take a non-const reference of a temporary object. You're trying to call operator<< with a temporary (A+B) but the signature of your overloaded operator<< takes a non-const reference so your function isn't a valid candidate.

To fix this you'll need to change a couple things. First, make your overloaded << operator take a const reference to the matrix. And don't forget to fix the friend signature in the class declaration.

std::ostream & operator<<(std::ostream &os, const matrix &M);

Just doing that will still cause errors because the element method is only defined for non-const objects. You'll need to add an overload for element that acts on const matrixes (and that won't let you modify the returned element).

double matrix::element(int i, int j) const
{
    int loc = (j-1) + (i-1)*columns;
    return array[loc];
}

Your operator<< and operator+ both need to take the input matrix by const reference, so that they can accept temporary matrix objects as input.

Also, operator+ should be const-qualified since it doesn't modify the contents of this. And element() should have a const -qualified overload so it can provide read-only access to a const matrix object.

You are also missing a copy constructor, a copy-assignment operator, a move constructor, and a move-assignment operator, per the Rule of 3/5/0. And also, your destructor is not freeing the C array at all, so it is being leaked.

Try this instead:

#include <iostream>
#include <stdexcept>
#include <utility>

class matrix {
    friend std::ostream& operator<<(std::ostream &os, const matrix &M);
private:
    int rows{}, columns{};
    double* array{nullptr};
public:
    matrix(int rows, int columns);
    matrix(const matrix &M);
    matrix(matrix &&M);
    ~matrix();

    matrix& operator=(matrix M);

    double& element(int r, int c);
    double element(int r, int c) const;

    matrix operator+(const matrix &M) const;
    matrix& operator+=(const matrix &M);
};

matrix::matrix(int r, int c)
    : rows(r), columns(c)
{
    size_t size = rows*columns;
    array = new double[size];
    for(size_t i = 0; i < size; ++i)
        array[i] = 0.0;
}

matrix::matrix(const matrix &M)
    : rows(M.rows), columns(M.columns)
{
    size_t size = rows*columns;
    array = new double[size];
    for(size_t i = 0; i < size; ++i)
        array[i] = M.array[i];
}

matrix::matrix(matrix &&M)
    : rows(M.rows), columns(M.columns), array(M.array)
{
    M.rows = M.columns = 0;
    M.array = nullptr;
}

matrix::~matrix()
{
    delete[] array;
}

matrix& operator=(matrix M)
{
    std::swap(rows, M.rows);
    std::swap(columns, M.columns);
    std::swap(array, M.array);
    return *this;
}

double& matrix::element(int r, int c)
{
    // TODO: do bounds checking here...
    size_t loc = ((r-1)*columns) + (c-1);
    return array[loc];
}

double matrix::element(int r, int c) const
{
    // TODO: do bounds checking here...
    size_t loc = ((r-1)*columns) + (c-1);
    return array[loc];
}

std::ostream & operator<<(std::ostream &os, const matrix &M)
{
    for (int r = 1; r <= M.rows; ++r){
        os << "[ ";
        for (int c = 1; c <= M.columns; ++c){
            os << M.element(r,c) << " "; 
        }
        os << "]\n";
    }

    /* alternatively...
    size_t loc = 0;
    for (int r = 0; r < M.rows; ++r){
        os << "[ ";
        for (int c = 0; c < M.columns; ++c){
            os << M.array[loc++] << " ";
        }
        os << "]\n";
    }
    */

    return os;
}

matrix matrix::operator+(const matrix &M) const
{
    matrix N(*this);
    N += M;
    return N;
}

matrix& matrix::operator+=(const matrix &M)
{
    if (M.rows != rows || M.columns != columns){
        throw std::runtime_error("DIMENSIONS OF MATRICES DO NOT MATCH");
    }

    for (int r = 1; r <= rows; ++r)
        for (int c = 1; c <= columns; ++c){
            element(r,c) += M.element(r,c);
        }
    }

    /* alternatively:
    size_t size = rows*columns;
    for (size_t i = 0; i < size; ++i)
        array[i] += M.array[i];
    }
    */

    return *this;
}

int main() {
    matrix A(2,2);
    matrix B(2,2);
    A.element(1,1) = 1;
    A.element(1,2) = 2;
    A.element(2,1) = 3;
    A.element(2,2) = 4;
    B.element(1,1) = 1;
    B.element(1,2) = 2;
    B.element(2,1) = 3;
    B.element(2,2) = 4;
    std::cout << A << std::endl;
    std::cout << B << std::endl;

    return 0;
}
Related