How can I take object from function?

Question

I wrote simple program to calculate some Matrix, and stuck at this problem. I can't get out my new Matrix.

This is my Matrix.h

#pragma once
using namespace std;
class Matrix
{
private:
    int row, col;
    float **matrix;
public:
    Matrix(int); // square matrix
    Matrix(int, int); // matrix with different rows and columns
    ~Matrix(); //delete
    void set(int, int, double); // set value to matrix
    double get(int, int); // get value from matrix
    void print(); // display matrix
    int rows(); // show rows
    int cols(); //show columns
    Matrix operator*(Matrix); // multiply matrix
};

#include <iostream>
#include <fstream>
#include "Matrix.h"
using namespace std;

Matrix::Matrix(int row) {
    if (row <= 0) {
        cout << "To small value for ROW or COL";
        exit(0);
    }
    this->row = row;
    this->col = row;
    this->matrix = new float* [row];

    for (int i = 0; i < row; i++)
    {
        this->matrix[i] = new float[row];

        for (int j = 0; j < row; j++)
        {
            this->matrix[i][j] = 0;
        }
    }
}

Matrix::Matrix(int row, int col) {
    if (row <= 0 || col <= 0) {
        cout << "To small value for ROW or COL";
        exit(0);
    }
    this->row = row;
    this->col = col;
    this->matrix = new float* [row];

    for (int i = 0; i < row; i++)
    {
        this->matrix[i] = new float[col];

        for (int j = 0; j < col; j++)
        {
            this->matrix[i][j] = 0;
        }
    }
}

Matrix::~Matrix() {
    for (int i = 0; i < this->row; i++)
    {
        delete[] matrix[i];
    }
    delete[] matrix;
}

int Matrix::rows() {
    return this->row;
}

int Matrix::cols() {
    return this->col;
}
void Matrix::set(int row, int col, double val) {
    if (row > this->row || col > this->col || row < 0 || col < 0) {
        cout << "There is no value to set.";
        exit(0);
    }
    else {
        this->matrix[row - 1][col - 1] = val;
    }
}
double Matrix::get(int row, int col) {
    if (row > this->row || col > this->col || row < 0 || col < 0) {
        cout << "There is no value, please correct ROW or COL.";
        exit(0);
    }
    else {
        cout << "Taken value from row " << row << " and col " << col << " = ";
        return this->matrix[row - 1][col - 1];
    }
}

void Matrix::print() {
    for (int i = 0; i < this->row; i++)
    {
        for (int j = 0; j < this->col; j++)
        {
            cout << this->matrix[i][j] << " ";
        }
        cout << endl;
    }
    cout << endl;
}

Matrix Matrix::operator*(Matrix B) {
        Matrix multiplied(B.row, this->col);

        for (int i = 0; i < this->row; i++) {
            for (int j = 0; j < B.col; j++) {
                multiplied.matrix[i][j] = 0;
                for (int k = 0; k < this->col; k++) {
                    multiplied.matrix[i][j] += this->matrix[i][k] * B.matrix[k][j];
                }
        }
        }
        multiplied.print(); // this work, and show correct answer in console
        return multiplied;
}

#include <iostream>
#include <fstream>
#include "Matrix.h"
using namespace std;
int main()
{  
    Matrix one(8,7), two(8,7), three(1,1), four(5);
    one.set(1, 1, 5);
    cout << one.get(1,5) << endl;
    one.print();
    two.set(1,2,2);
    two.print();
    Matrix nine = two * one; // but two*one from Matrix.cpp gives me correct Matrix
    nine.print(); // this stop the program
}

I'm getting something like this: makefile:4: recipe for target 'run' failed make: *** [run] Segmentation fault (core dumped)

I'm using makefile to run this code. Where is the problem?

Answer 1

A comment on your question pinpoints the reason why things are failing - You did not define a copy constructor, so the compiler implicitly defines one for you. The implicit copy constructor copies your matrix pointer into a new Matrix, and then it gets deleted both inside operator*() call and outside where the new matrix is returned. You can fix it by defining copy and move constructors, but I think there's something much better.

First of all, if you find yourself using new and delete manually in your code, it's usually wrong in modern C++. Secondly, you don't need to allocate new arrays for each row - they are all going to be the same size, and you can do one single allocation.

#pragma once

#include <vector>

// never use namespace aliases in header files - this polutes the global namespace of every user.

class Matrix
{
private:
    size_t rows; // unsigned integers prevent values less than 0
    size_t cols;
    std::vector<double> elements;
public:
    Matrix(size_t);
    Matrix(size_t, size_t);
    // destructor no longer needed! vector handles it
    void set(size_t, size_t, double);
    double get(size_t, size_t) const; // follow const correctness
    void print();
    size_t rows();
    size_t cols(); 
    Matrix operator*(Matrix);
};

Some implementations:

// This is the whole constructor. Vector will zero-initialize all the values
Matrix::Matrix(size_t row, size_t col)
    : rows(row)
    , cols(col)
    , elements(row * col)
{ }

void Matrix::set(size_t row, size_t col, double val) {
    elements[row * cols + col] = val;
}
double Matrix::get(size_t row, size_t col) const {
    return elements[row * cols + col];
}