Problem involving pointers and manually implemented matrix classes

Question

I'm currently working on a bigger project which involves implementing a linear algebra calculator. I have decided to not use any other already-existing libraries which might help me implement it as I thought it would be too easy.

I first started to code the Matrix class, which now looks like this:

class Matrix{
private:
    int rows; // no. rows
    int columns; // no. columns
    double** matVal; //values of the matrix
    char name; //only used when printing it out or by outside programs.
public:
    //constructors and destructor
    Matrix();
    Matrix(int r,int c,char _name);
    ~Matrix();

    //basic get functions for private access
    int getNrRows();
    int getNrCols();
    char getName();
    double getVal(int row,int col);

    //basic set functions for private variables
    void setVal(int row,int col,double value);
    void setName(char _name);

    //basic matrix operations
    Matrix operator=(Matrix M);
    Matrix operator+(Matrix M);
    Matrix operator-(Matrix M);
    Matrix operator*(Matrix M);

    //Printing out the matrix
    void Print();
};

At first it went smoothly, but then I stumbled into a fatal bug which wont allow me to progress further. For more information, here are my functions ( + some code to try to figure out what's wrong) and what I executed in main():

#define cout std::cout

Matrix::Matrix(){
    rows = 0;
    columns = 0;
    matVal = nullptr;
}

Matrix::Matrix(int r,int c,char _name){
    rows = r;
    columns = c;
    name = _name;
    matVal = new double*[r];
    for(int i = 0; i < r; i++){
        matVal[i] = new double[c];
    }
    for(int i = 0; i < r; i++){
        for(int j = 0; j < c; j++){
            matVal[i][j] = 0;
        }
    }
}

Matrix::~Matrix(){
    for (int i = 0; i < rows; i++)
        delete[] matVal[i];
    delete[] matVal;
}

int Matrix::getNrRows(){
    return rows;
}

int Matrix::getNrCols(){
    return columns;
}

char Matrix::getName(){
    return name;
}

double Matrix::getVal(int row, int col){
    return matVal[row-1][col-1];
}

void Matrix::setVal(int row,int col,double value){
    matVal[row-1][col-1] = value;
}

void Matrix::setName(char _name){
    name = _name;
}

Matrix Matrix::operator=(Matrix M){
    for (int i = 0; i < rows; i++)
        delete[] matVal[i];
    delete[] matVal;

    rows = M.rows;
    columns = M.columns;

    matVal = new double*[rows];

    for(int i = 0; i < rows; i++){
        matVal[i] = new double[M.columns];
    }

    for(int i = 0; i < M.rows; i++){
        for(int j = 0; j < M.columns; j++){
            matVal[i][j] = M.matVal[i][j];
            cout<<matVal[i][j]<<' ';
        }
        cout<<'\n';
    }
    cout<<this<<std::endl;
    return *this;
}

Matrix Matrix::operator+(Matrix M){
    Matrix Rez;
    Rez.rows = rows;
    Rez.columns = columns;
    for(int i = 0; i < rows; i++){
        for(int j = 0; j < columns; j++){
            Rez.matVal[i][j] = matVal[i][j] + M.matVal[i][j];
        }
    }
    return Rez;
}

void Matrix::Print(){
    cout<<'\n';
    cout<<name<<": "<<"\n";
    for(int i = 0; i < rows; i++){
        for(int j = 0; j < columns; j++){
            cout<<matVal[i][j]<<' ';
        }
        cout<<'\n';
    }
    cout<<'\n';
    return;
}

Main:

Matrix M(4,3,'A');
M.setVal(1,1,2);
M.setVal(1,3,-1.1);
M.Print();
Matrix A(4,3,'B');
A.setVal(3,2,5);
A.Print();
Matrix C(4,3,'C');
C = A;
cout<<C.getVal(3,2)<<'\n';
cout<<C.getNrCols()<<" "<<C.getNrRows()<<endl;
C.Print();
cout<<"S"<<endl;

Printing the first 2 matrices works fine, when I print each element of C in the operator= function after I assigned it the proper value, again, it works fine, but when I use the Print() function on C it crashes. Here is the console output for the code above:

A:
2 0 -1.1
0 0 0
0 0 0
0 0 0


B:
0 0 0
0 0 0
0 5 0
0 0 0

0 0 0
0 0 0
0 5 0
0 0 0
0x69fed0
5
3 4

C:

At first I had absolutely no idea why it did that, but then I printed the pointer to each variable instead(it printed it all and returned 0 this time):

A:
0x850e38 0x850e40 0x850e48
0x851318 0x851320 0x851328
0x851338 0x851340 0x851348
0x851358 0x851360 0x851368


B:
0x851390 0x851398 0x8513a0
0x8513b0 0x8513b8 0x8513c0
0x8513d0 0x8513d8 0x8513e0
0x855b08 0x855b10 0x855b18

0x855b40 0x855b48 0x855b50
0x855b60 0x855b68 0x855b70
0x855b80 0x855b88 0x855b90
0x855ba0 0x855ba8 0x855bb0
0x69fed0
5
3 4

C:
0 0x8 0x10
0 0x8 0x10
0 0x8 0x10
0 0x8 0x10

S

Now I think that there is a problem with the Print function (because otherwise why would I be able to get the 5 printed out in main?). I still don't know exactly what is happening so I ask for your help. I'm sorry if this is a rookie mistake, I'm still quite inexperienced.

I also forgot to add that the class and class functions are in separate files (header and cpp), although I don't know how this could affect things.

Answer 1

The signature Matrix operator=(Matrix&); proposed in another answer it totally wrong. The correct signature should be

Matrix& operator=(const Matrix&);

or

void operator=(const Matrix&);

if you don't need chaining of assignments (a = b = c).

The bare minimum that you have to implement: copy constructor, copy assignment, and destructor. For a matrix class it is also reasonable to implement move operations. This is known as the rule of zero/three/five (it is five in our case):

If a class requires no user-defined constructors, no user-defined assignment operators and no user-defined destructor, don't define them; if a class requires a user-defined destructor, a user-defined copy (and move) constructor, or a user-defined copy (and move) assignment operator, it almost certainly requires all three (five).

Let's assume that matrix internally is represented as a 1D array. This approach avoids unnecessary indirection for matrix element access and simplifies code.

class Matrix {
public:
    Matrix(const Matrix&);
    Matrix(Matrix&&);

    Matrix& operator=(const Matrix&);
    Matrix& operator=(Matrix&&);

    ~Matrix();

private:
    double* data_        = nullptr;
    std::ptrdiff_t rows_ = 0;
    std::ptrdiff_t cols_ = 0;
};

Copy operations should be deep, i.e. they should copy data, not just underlying pointer. Let's start with a copy constructor. It should allocate storage, then copy data from other to this storage:

Matrix(const Matrix& other) 
: rows_(other.rows_), cols_(other.cols_) {
    const auto n = other.rows_ * other.cols_;
    data_ = new double[n];
    std::copy(other.data_, other.data_ + n, data_);
}

Now let's implement swap:

void swap(Matrix& other) {
    std::swap(rows_, other.rows_);
    std::swap(cols_, other.cols_);
    std::swap(data_, other.data_);
}

This function is very useful, because it allows us to implement move constructor, copy assignment and move assignment with almost no code:

Matrix(Matrix&& other) {
    swap(other);
}

Matrix& operator=(const Matrix& other) {
    Matrix(other).swap(*this);
    return *this;
}

Matrix& operator=(Matrix&& other) {
    Matrix(std::move(other)).swap(*this);
    return *this;
}

With such canonical implementations you can be sure that these functions are implemented correctly (including self-assignment handling) once you have correct implementations of copy constructor and swap. Appreciate the elegance of the copy-and-swap idiom.

Now let's talk about operator+. Take a look at this implementation:

Matrix operator+(Matrix other) const {
    assert(rows_ == other.rows_);
    assert(cols_ == other.cols_);

    const auto n = rows_ * cols_;
    for (std::ptrdiff_t i = 0; i < n; ++i)
        other.data_[i] += data_[i];
    return other;
}

Here we take the argument by value and get a (deep) copy of it. Then we add this->data_ to the copy and return that copy. There is no need to introduce another local Matrix variable.

Complete demo

Answer 2

you can change the operator= function to this form:

Matrix operator=(Matrix &M);

and in definition you should change its return to:

return M;

i checked this and worked. in operator+(and *) consider this note too.