Dodawanie,odejmowanie i mnożenie macierzy - dziwne wyniki

Question

Problem mam taki, że muszę zrobić dodawanie, odejmowanie i mnożenie macierzy i wydaje mi się że mam wszystko tak jak trzeba a po kompilacji wyświetlają się cuda, tzn pierwsze dwie kolumny to jakieś randomowe liczby natomiast trzecia kolumnia wydaje sie być w porządku... Kompiluje to na linuxie w terminalu. Jakby mógł ktoś zerknąć na kod i podpowiedzieć w czym jest problem to byłbym wdzięczny ;) pozdro!

 

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



class Matrix{

	private:
		
    int rows;
    int columns;
    int **matrix;
    
	public:

   	int getRows();

   	int getColumns();
    
    	void setRows(int x);
    
    	void setColumns(int x);
    
    	void set(int n, int m, int val);
    
    	int get(int n, int m);

    	void toString();

    	int getMatrix();

	Matrix add(Matrix m2);
	
	Matrix subtract(Matrix m2);
	
	Matrix multiply(Matrix m2);
	
    	Matrix ( int row, int column);

   	~Matrix();
   	};

#include "Functions.h"
#include <iostream>
#include <cstdlib>
#include <stdio.h>


using namespace std;


Matrix::Matrix(int row, int column) {
    this->rows = row;
    this->columns = column;

    //alokacja pamieci na tablice
    matrix = new int *[row];
    for (int i = 0; i < row; i++)
        matrix[i] = new int[column];

    //wypelnianie macierzy
    for (int i = 0; i < row; i++)
        for (int j = 0; j < column; j++)
            matrix[i][j] = 0;

};

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

};

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

int Matrix::getColumns() {
    return this->columns;
};

void Matrix::setColumns(int x){
	columns = x;
};

void Matrix::setRows(int x)
{
	rows = x;
};



void Matrix::set (int n, int m, int val) {
     matrix[n][m]= val;
};

int Matrix::get (int n, int m){
     return matrix[n][m];
     };

void Matrix::toString() {
    for ( int i = 0 ; i < rows; i++) {
        cout << "\n";
        for ( int j = 0; j < columns; j++)
            cout << matrix[i][j] << "\t";
    }
};

Matrix Matrix::add (Matrix m2)
{
	Matrix m3(m2.getColumns(), m2.getRows() );
	int val = 0;
		for( int i = 0; i < m2.getColumns(); i++) {
			for( int j = 0; j < m2.getRows(); j++) {
			val = matrix[i][j] + m2.get(i,j);
			m3.set(i , j , val);
			val = 0;
		}
	}		

	return m3;
};

Matrix Matrix::subtract (Matrix m2){
       int val=0;
       Matrix m3( m2.getColumns() , m2.getRows() );
       for(int i = 0; i <  m2.getColumns(); i++) {
		for(int j = 0; j <  m2.getRows(); j++) { 
                  val = matrix[i][j] - m2.get(i,j);
                     m3.set( i , j , val );
                     val=0;
			}
		 }
		return m3;
};

Matrix Matrix::multiply(Matrix m2){
        int val=0;
       Matrix m3( m2.getColumns() , m2.getRows() );
       for(int i = 0; i <  m2.getColumns(); i++) {
			for(int j = 0; j <  m2.getRows(); j++) { 
				m3.set(i,j,0);
				for( int g = 0 ; g < m2.getColumns() ; g++){
                  val = m3.get(i,j) + matrix[i][g] * m2.get(g,j);
                     m3.set(i,j,val);
                     val=0;
                 }
			}
		 }
		return m3;
       }





int Matrix::getMatrix()
{
    return **matrix;
}

#include <iostream>
#include <cstdlib>
#include <stdio.h>
#include "Functions.cpp"

 
using namespace std;


int main() {
	
	
	Matrix m1(3,3);
	Matrix m2(3,3);
	Matrix m3(3,3);
	
	m1.set(0,0,2);
	m1.set(0,1,2);
	m1.set(0,2,2);
	m1.set(1,0,2);
	m1.set(1,1,2);
	m1.set(1,2,2);
	m1.set(2,0,2);
	m1.set(2,1,2);
	m1.set(2,2,2);
	
	m2.set(0,0,1);
	m2.set(0,1,1);
	m2.set(0,2,1);
	m2.set(1,0,1);
	m2.set(1,1,1);
	m2.set(1,2,1);
	m2.set(2,0,1);
	m2.set(2,1,1);
	m2.set(2,2,1);

	m1.toString();	
	cout<<endl;
	m2.toString();	
	m3 = m1.add(m2);
	m3.toString();	
	cout<<endl;
	
	
}

 

Comments

Twój kod jest mało czytelny. Dlaczego piszesz wszystko sam, a nie korzystasz z dobrodziejstw STL?

Answer 1

tl;dr domyślny operator przypisania robi płytką kopię dwuwymiarowej tablicy int** matrix, więc odwołujesz się do zwolnionej pamięci, bo nie przestrzegasz zasady trzech (lub pięciu).

Matrix Matrix::add(Matrix& m2)
{
    Matrix m3(m2.getColumns(), m2.getRows());
    ...
    return m3;
}
...
m3 = m1.add(m2);

Tutaj dzieje się o wiele więcej niż myślisz.

Zwracasz m3 przez wartość:

• Stworzona zostanie kopia m3 przy pomocy domyślnego konstruktora przenoszącego Matrix(Matrix&&), który skopiuje Ci wszystkie pola do nowego obiektu, łącznie ze wskaźnikiem matrix. Mówimy o płytkiej kopi. Skopiowana zostanie wartość adresu, nie pamięć pod tym adresem.
• m3 zostanie zniszczone ~Matrix (który tak na marginesie zwalnia pamięć pod wskaźnikiem matrix, do której odwołuje się nowy obiekt, będący wynikiem kopi z poprzedniego kroku)

Te dwa kroki mogą być pominięte w przypadku NRVO.

m3 = m1.add(m2);

Zostaje wywołany domyślnie zdefiniowany przenoszący operator przypisania Matrix& operator=(Matrix&&) a zaraz po nim destruktor tymczasowej kopi zwróconej z funkcji add (lub oryginalnego m3 z funkcji add, w przypadku pominięcia kopii). Ponownie, domyślny operator przypisania robi płytką kopię wszystkich pól, więc w efekcie trzymasz wskaźnik do dwuwymiarowej tablicy matrix zwolnionej już dwukrotnie (lub jednokrotnie jeśli kopia była pominięta).

Dlatego właśnie w m3 z maina masz śmieci. Odwołujesz się do zwolnionej pamięci.

Jak temu zaradzić. Numer jeden - nie bawić się w gołe wskaźniki. std::vector w zupełności tu wystarczy.

Numer 2 - zasada trzech lub pięciu. Jeśli operujesz na wskaźnikach, to musisz implementować operatory przypisania oraz konstrktory kopiujące, tak aby tworzyły one kopie głębokie a nie płytkie.

Kopia głęboka zaalokowałaby nową tablice 2d i skopiowała jej wartości. Kopia płytka po prostu przypisuje wartość wskaźnika. Mamy więc dwa wskaźniki do tej samej tablicy 2d.

Tak. C++ to nie jest fajny język dla początkujących :)