ポインタを戻り値として返す方法

Question

#戻り値でポインタを含むオブジェクトを返したい
C++で行列クラスの作成をしています．いま，operatorを用いて行列同士の加算を+で定義しようとしたところ，コンパイルは通ったのですが，実行結果が正常に動作しませんでした．operatorの戻り値にポインタが含まれているため正常に動作しないということはわかったのですが，どのように訂正すれば良いかわかりません．どなたか教えてください...

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

// 行列クラス
class Matrix{
protected:
    double *m_matrix;
    int m_row;
    int m_column;
public:
    Matrix(int R, int C);   //行列のサイズを指定して零行列を作成
    Matrix(int R, int C, double *V);    //行列のサイズを指定して行列を作成
    Matrix(Matrix &matrix); //引数に指定された行列と同じ値を持つ新しい行列を生成
    ~Matrix();  //コンストラクタで確保した動的メモリを解放
    int IsOK();           //行列が有効かどうか返す
    void Show();                //格納されている行列を画面に表示する
    Matrix operator+ (Matrix &mat1); //２つの行列を加算
};

//行列のサイズを指定して零行列を作成
Matrix::Matrix(int R, int C) : m_matrix(NULL)
{
    if(R<1){
        fprintf(stderr,"行指定が不正です.\n");
        return ;
    }
    if(C<1){
        fprintf(stderr,"列指定が不正です.\n");
        return ;
    }
    
    m_row=R;
    m_column=C;
    m_matrix = new double [m_row*m_column];
    
    for(int i=0; i<m_row*m_column; i++){
        m_matrix[i]=0;
    }
    
}

//行列のサイズを指定して行列を作成
Matrix::Matrix(int R, int C, double *V) : Matrix(R, C)
{
    if(m_matrix == NULL)  return;
    
    for (int i=0; i<R*C; i++){
        m_matrix[i]=V[i];
    }
}
//引数に指定された行列と同じ値を持つ新しい行列を生成(複製コンストラクタ)
Matrix::Matrix(Matrix &matrix1):Matrix(matrix1.m_row,matrix1.m_column)
{
    if (m_matrix == NULL) return;
    if (matrix1.m_matrix == NULL) return ;
    int N=matrix1.m_row*matrix1.m_column;
    m_row=matrix1.m_row;
    m_column=matrix1.m_column;
    m_matrix=new double [N];
    for(int i=0; i<m_row*m_column; i++){
        m_matrix[i]=matrix1.m_matrix[i];
    }
}
//コンストラクタで確保した動的メモリを解放
Matrix::~Matrix()
{
    delete [] m_matrix;
    m_matrix = NULL;
}

//行列がNULLかどうか返す
//不正な計算が行われた際行列にNULLを代入している
//行列がNULLの時画面表示しないようにIsOkをShow()関数内で呼び出す
int Matrix::IsOK()
{
    if (m_matrix == NULL)
        return 0;
    else
        return 1;
}
//格納されている行列を画面に表示する
//配列がNULLなら画面表示しないようにする
void Matrix::Show()
{
    if(IsOK() == 0){
        printf("ERROR\n");
        return;
    }
    for(int i=0; i<m_row; i++){
        printf("|");
        int j;
        for(j=0; j<m_column; j++){
            printf("%6.2f",m_matrix[i*(m_column)+j]);
            if(j<m_column-1)
                printf(" ");
        }
        printf("|\n");
    }
}


//２つの行列を加算
//エラー
Matrix Matrix::operator+ (Matrix &mat1)
{
    Matrix mat(m_row,m_column);
    for(int i=0; i<m_row*m_column; i++){
        mat.m_matrix[i] = m_matrix[i];
        mat.m_matrix[i] += mat1.m_matrix[i];
    }
    return mat;
}

int main()
{
    double val1[]={1.0,2.0,3,0,4,0,5.0,6.0};
    Matrix mat1(2,3,val1);
    printf("mat1=\n");
    mat1.Show();
    double val2[]={0.0,2.0,3.0,4,0,5.0,6.0};
    Matrix mat2(2,3,val2);
    printf("mat2=\n");
    mat2.Show();
    Matrix mat3(mat1);
    mat3=mat1+mat2;
    printf("mat3=\n");
    mat1.Show();
}

実行結果::
mat1=
|  1.00   2.00   3.00|
|  0.00   4.00   0.00|
mat2=
|  0.00   2.00   3.00|
|  4.00   0.00   5.00|
mat3=
|  1.00   2.00   3.00|
|  0.00   4.00   0.00|
*** Error in `': double free or corruption (fasttop): 0x000000000207bd20 ***
======= Backtrace: =========

======= Memory map: ========

Answer 1

こんにちは。

ポインタの多重開放が発生していると思います。
Matrix Matrix::operator+ (Matrix &mat1)の中で mat を獲得していますが、これは文法上は operator+を抜けた時点で開放されます。従って、mat.m_matrixもoperator+から抜ける時に~Matrixにてdelete[]されます。

そして、operator=を定義していないのに使っているため、このケースではコンパイラがoperator=(Matrix const&)を自動生成します。自動生成なのでそれ程高度なものではなく、単にメンバ変数同士を代入するだけです。m_matrixもmemcpy的にコピーされるのではなく単純にアドレス値が代入されるだけです。
ということは、main()関数のmat3=mat1+mat2;にて、mat.m_matrixがmat3.m_matrixへ上書きされてしまいます。この時点で、元から有ったmat3.m_matrixが指す領域がリークします。
次に、mat3はmain()関数の終わりで開放されます。そこでもmat3.m_matrixがdelete[]されますが、これは既にoperator+から抜ける時に開放されていますから多重開放となり、問題のエラーとなります。

対策はコピー代入演算子を定義すると良いです。

Matrix& operator=(Matrix const& matrix1)
{
    int N=matrix1.m_row*matrix1.m_column;
    m_row=matrix1.m_row;
    m_column=matrix1.m_column;
    m_matrix=new double [N];
    for(int i=0; i<m_row*m_column; i++){
        m_matrix[i]=matrix1.m_matrix[i];
    }
    return *this;
}

ここで、Matrix const& matrix1のconstが意外に重要なのでご注意下さい。
mat3=mat1+mat2;の右辺は一時オブジェクトを生成し、mat3のoperator=(Matrix const&)でそれをコピーします。しかし、Matrix& matrix1は一時オブジェクトを受け取れないとC++規格で決められていますので多くのコンパイラでエラーになります。（何故ダメ？って感じがしますし、実際ちょっと古いmsvcでは通りますので混乱しやすい部分です。）
constについては以前解説したことがありますので、良かったら参考にされて下さい。

ところで、最後にmat3=としてmat1を出力してますよ。
ハマってデバッグしたので、Show()とIsOk()にconstを補ってデバッグしやすいようにしました。

Comments

[tkhs314]

丁寧に解答してくださりありがとうございます！
リンク先も読ませていただきました．
2点質問があります．
参照をつけると"ポインタのように戻り値を介さずに値を書き換えられるようにできる"という認識なのですが，それがconst と矛盾するような気がしてしまいます.
参照を用いるのはどのような時なのでしょうか?
また，const& matとconst &matでは何か意味は変わってしまうのでしょうか?

[Chironian]

参照やポインタをパラメータとして渡す目的はもう一つ、コピーしないで大量のデータを渡すことがあります。そして、受け取った側がそのパラメータの中身を修正しないのであれば、修正しないことをコンパイラに教え、間違って修正したらコンパイラに教えて貰うという使い方ができます。これによりバグが減り、かつ、テスト工数も減ります。
const& matとconst &matは同じです。私は型を示すという雰囲気がでるので「Matrix const&」の方を好みますが、この辺は好みの問題と思います。

Answer 2

    Matrix mat3(mat1);
    mat3=mat1+mat2;

これを実現するためにはoperator=を再定義し
mat3.m_matrixを解放し、(mat1+mat2).m_matrixの内容をコピーする必要があります。

こんな感じ

Matrix& Matrix::operator=(const Matrix &other){
    delete[] m_matrix;
    m_row = other.m_row;
    m_column = other.m_column;
    m_matrix = new double[m_row * m_column];
    for(int i=0; i<m_row*m_column; i++){
        m_matrix[i]=other.m_matrix[i];
    }
    return *this;
}

---

もしくはさらに、ムーブ代入を追記しておくと
コピーが不要になり効率がよくなります

Matrix& Matrix::operator=(Matrix &&other){
    delete[] m_matrix;
    m_row = other.m_row;
    m_column = other.m_column;
    m_matrix = other.m_matrix;
    other.m_matrix = nullptr;
    return *this;
}