Gründe für malloc NULL zu returnen

Wie oenone sehe ich jetzt in diesem "Beispiel" auch nicht den Grund für Deine Probleme.

Allerdings frage ich mich, warum Du so einen eigensinnigen Stil entwickeln willst. malloc() / free() , die ganzen const in der Schnittstelle...

Das mit dem memset() macht übrigens nur, das was Du willst, wenn die Repräsentation von 0.0 als double auch nur aus lauter 0 Bytes besteht (Was aber zu Deinem Glück bei IEEE754 der Fall ist - jedenfalls +0.0)

Wo ist denn da eigentlich der Kopierkonstruktor? Und wie hast Du den Zuweisungsoperator implementiert?

DragonRaider

Hallo,
OK, danke. Dann werde ich mal meinen Sourcecode senden... Es wäre sehr nett, würde sich jemand das ganze mal ansehen. Danke schon mal im Voraus

mat.h:

#ifndef MATRIX_LIBRARY_H
#define MATRIX_LIBRARY_H

#include <opencv2/core/core.hpp>                //Das kann eigentlich auch raus, ist noch Altlast
#include <opencv2/highgui/highgui.hpp>
#include <opencv2/imgproc/imgproc.hpp>

class Matrix
{
public:
    Matrix(unsigned const int m, unsigned const int n);                //m = rows, n = columns
    ~Matrix();

    unsigned const int getN();
    unsigned const int getM();

    Matrix operator*(Matrix b);
    double *operator[](unsigned const int i);
    Matrix& operator=(Matrix other);

    void print();

private:
    unsigned int _n, _m;
    double *_data;
};

#endif

mat.cpp:

#include "mat.h"
#include <stdexcept>
#include <cstdlib>
#include <cstring>
#include <iostream>

Matrix::Matrix(unsigned const int m, unsigned const int n) : _n(n), _m(m)
{
    std::cout << "    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)" << std::endl;
    _data = new double[_m * _n];
    std::cout << "    Allocated double* _data" << std::endl;
    memset(_data, 0, sizeof(double) * _m * _n);
    std::cout << "    Set values to 0" << std::endl;
}

Matrix::~Matrix()
{
    //free(_data);
    delete [] _data;
}

unsigned const int Matrix::getN()
{
    return _n;
}

unsigned const int Matrix::getM()
{
    return _m;
}

Matrix& Matrix::operator=(Matrix other)
{
    if(this != &other)
    {
        double *tmp_data = new double[other.getM() * other.getN()];
        std::cout << "    Allocated double* tmp_data" << std::endl;
        if(tmp_data == NULL)
        {
            std::cerr << "tmp_data == NULL" << std::endl;
            throw std::bad_alloc();
        }

        for(unsigned int i = 0; i < other.getM(); ++i)
        {
            for(unsigned int j = 0; j < other.getN(); ++j)
            {
                tmp_data[i * other.getN() + j] = *other[i * other.getN() + j];
            }
        }
        std::cout << "    Updated double* tmp_data" << std::endl;

        _m = other.getM();
        _n = other.getN();
        std::cout << "    Updated double _m and double _n" << std::endl;

        delete [] _data;
        std::cout << "    Freeed double* _data" << std::endl;
        _data = tmp_data;
        std::cout << "    Updated double* _data" << std::endl;
    }

    return *this;
}

double *Matrix::operator[](unsigned const int i)
{
    return &_data[i * _n];
}

Matrix Matrix::operator*(Matrix b)
{
    std::cout << "  Entered Matrix Matrix::operator*(Matrix b)" << std::endl;
    Matrix result(this->getM(), b.getN());
    std::cout << "  Created Matrix result" << std::endl;

    if(this->getN() != b.getM())
    {
        std::cerr << "Invalid Matrix size." << std::endl;
        throw std::logic_error("Invalid Matrix size.");
    }
    std::cout << "  Checked size of Input Matrize" << std::endl;

    for(unsigned int i = 0; i < this->getM(); ++i)
    {
        for(unsigned int j = 0; j < b.getN(); ++j)
        {
            double value = 0.0;
            for(unsigned int index = 0; index < b.getM(); ++index)
            {
                value += (*this)[i][index] * b[index][j];
            }
            result[i][j] = value;
        }
    }

    std::cout << "  Returning Matrix result" << std::endl;
    return result;
}

void Matrix::print()
{
    for(unsigned int i = 0; i < _m; ++i)
    {
        for(unsigned int j = 0; j < _n; ++j)
        {
            std::cout << (*this)[i][j];
            if(j < _n - 1)
            {
                std::cout << " | ";
            }
        }
        std::cout << std::endl;
    }
}

main.cpp:

#include "mat.h"
#include <exception>
#include <iostream>
#include <string>
#include <time.h>
#include <opencv2/core/core.hpp>
#include <opencv2/highgui/highgui.hpp>
#include <opencv2/imgproc/imgproc.hpp>
using namespace cv;

#define DEBUG
#define DEBUG2

std::string paths[9] =  {"D:\\Matrix\\Pictures\\05.jpg",
                         "D:\\Matrix\\Pictures\\11.jpg",
                         "D:\\Matrix\\Pictures\\03.jpg",
                         "D:\\Matrix\\Pictures\\04.jpg",
                         "D:\\Matrix\\Pictures\\02.jpg",
                         "D:\\Matrix\\Pictures\\06.jpg",
                         "D:\\Matrix\\Pictures\\07.jpg",
                         "D:\\Matrix\\Pictures\\08.jpg",
                         "D:\\Matrix\\Pictures\\10.jpg"};

double dRand(double dMin, double dMax)
{
    double d = (double)rand() / RAND_MAX;
    return dMin + d * (dMax - dMin);
}

int main()
{
    srand(time(NULL));

    Matrix A(35 * 3, 15 * 3);
    Matrix B(15 * 3, 1);
    Matrix C(35 * 3, 1);

    Matrix ReferenceValue(35 * 3, 1);

    for(unsigned int i = 0; i < 35 * 3; ++i)
    {
        for(unsigned int j = 0; j < 15 * 3; ++j)
        {
            A[i][j] = dRand(0.0, 0.02);
        }
    }

    namedWindow("Full Size Image", WINDOW_AUTOSIZE);

    namedWindow("Original Image Section", WINDOW_NORMAL);
    resizeWindow("Original Image Section", 7 * 50, 5 * 50);

    namedWindow("Downscaled Image Section", WINDOW_NORMAL);
    resizeWindow("Downscaled Image Section", 5 * 50, 3 * 50);

    namedWindow("Output", WINDOW_NORMAL);
    resizeWindow("Output", 7 * 50, 5 * 50);

    for(unsigned int index = 0; index < 9; ++index)
    {
        Mat image = imread(paths[index], CV_LOAD_IMAGE_COLOR);
        imshow("Full Size Image", image);

        Mat ToScaleDown(5, 7, image.type());

        std::cout << "Rows: " << image.rows << ", Cols: " << image.cols << std::endl;

        for(unsigned int x = 0; x < image.cols - 7; ++x)
        {
            for(unsigned int y = 0; y < image.rows - 5; ++y)
            {
                for(unsigned int xSection = 0; xSection < 7; ++xSection)
                {
                    for(unsigned int ySection = 0; ySection < 5; ++ySection)
                        {
                        //Updaten der ReferenceValue Matrix
                        }
                }

                std::cout << "Updated Matrix ReferenceValue" << std::endl;

                ToScaleDown = image(Range(y, y + 5), Range(x, x + 7));
                std::cout << "Loaded cv::Mat ToScaleDown" << std::endl;
                imshow("Original Image Section", ToScaleDown);
                resize(ToScaleDown, ToScaleDown, cvSize(5, 3), 0, 0, INTER_AREA);
                std::cout << "Resized cv::Mat ToScaleDown" << std::endl;
                imshow("Downscaled Image Section", ToScaleDown);

                for(unsigned int i = 0; i < 3; ++i)
                {
                    for(unsigned int j = 0; j < 5; ++j)
                    {
                        //Updaten der Matrix B
                    }
                }
                std::cout << "Updated Matrix B" << std::endl;

                C = A * B;
                std::cout << "Calculated Matrix C" << std::endl;

                Mat OutputMat(5, 7, image.type());
                for(unsigned int i = 0; i < 5; ++i)
                {
                    for(unsigned int j = 0; j < 7; ++j)
                    {
                        //Matrix C -> cv::Mat OutputMat
                    }
                }
                std::cout << "Generated cv::Mat OutputMat" << std::endl;
                imshow("Output", OutputMat);

                for(unsigned int i = 0; i < 35 * 3; ++i)
                {
                    //Bearbeiten der Matrix A
                }
                std::cout << "Matrix A Re-Calculated" << std::endl;
                std::cout << std::endl;
                cvWaitKey();
            }
            std::cout << "One Column - " << x << std::endl;
        }
    }
}

Das Log bist zum Absturz sieht dann wie folgt aus:

Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
Rows: 747, Cols: 1023
Updated Matrix ReferenceValue
Loaded cv::Mat ToScaleDown
Resized cv::Mat ToScaleDown
Updated Matrix B
  Entered Matrix Matrix::operator*(Matrix b)
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)
    Allocated double* _data
    Set values to 0
  Created Matrix result
  Checked size of Input Matrize                     //Schlechtes Englisch ^^
  Returning Matrix result
    Allocated double* tmp_data
    Updated double* tmp_data
    Updated double _m and double _n
    Freeed double* _data
    Updated double* _data
Calculated Matrix C
Generated cv::Mat OutputMat
Matrix A Re-Calculated

Updated Matrix ReferenceValue
Loaded cv::Mat ToScaleDown
Resized cv::Mat ToScaleDown
Updated Matrix B
  Entered Matrix Matrix::operator*(Matrix b)
    Entered Matrix::Matrix(unsigned const int m, unsigned const int n)

MfG DragonRaider

Edit 1: Ich hatte mal gelesen, es wäre sauberer sich Argumente als const übergeben zu lassen, wenn man sie in der Funktion nicht verändern möchte
Edit 2: Danke, das mit memset hatte sich mir beim Lesen der Referenz auch schon erschlossen. Aber ich war einfach mal davon ausgegangen, dass man 0 halt mit nur Nullen "darstellt"
Edit 3: Hahaha in der main.cpp fehlte ein Stück ^^

Das ganze brauche ich mir gar nicht ansehen.
Der fehlende Kopierkonstruktor ist schon mal ein schöner showstopper.

Sobald Du eine Kopie anfertigst (z.b. das Argument von Matrix& operator=(Matrix); ), fliegt Dir das doch alles um die Ohren.

Schau Dir nochmal Referenzen als Typen von Funktionsargumenten an. Also "by value" vs. "by reference".

Lies mal, was man unter der Rule of Three in C++ versteht und warum Dich das betrifft.

DragonRaider

Vielen Dank ich habe mir den Artikel zur Rule of Three noch ein mal durch gelesen und daraufhin den Copykonstruktor und, soweit mir das logisch vor kam noch ein mal die const neu gesetzt. Jetzt sieht meine Klasse wie folgt aus:

mat.h:

#ifndef MATRIX_LIBRARY_H
#define MATRIX_LIBRARY_H

class Matrix
{
public:
    Matrix(unsigned int m, unsigned int n);                //m = rows, n = columns
    Matrix(const Matrix &other);
    ~Matrix();

    unsigned int getN() const;
    unsigned int getM() const;

    Matrix operator*(const Matrix b) const;
    double *operator[](unsigned const int& i) const;
    Matrix& operator=(Matrix other);

    void print();

private:
    unsigned int _n, _m;
    double *_data;
};

#endif

mat.cpp

#include "mat.h"
#include <stdexcept>
#include <cstdlib>
#include <cstring>
#include <iostream>

Matrix::Matrix(unsigned int m, unsigned int n) : _n(n), _m(m)
{
    _data = new double[_m * _n];
    memset(_data, 0, sizeof(double) * _m * _n);
}

Matrix::Matrix(const Matrix& other)
{
    double *tmp_data = new double[other.getM() * other.getN()];

    for(unsigned int i = 0; i < other.getM(); ++i)
    {
        for(unsigned int j = 0; j < other.getN(); ++j)
        {
            tmp_data[i * other.getN() + j] = *other[i * other.getN() + j];
        }
    }

    _m = other.getM();
    _n = other.getN();

    delete [] _data;
    _data = tmp_data;
}

Matrix::~Matrix()
{
    delete [] _data;
}

unsigned int Matrix::getN() const
{
    return _n;
}

unsigned int Matrix::getM() const
{
    return _m;
}

Matrix& Matrix::operator=(Matrix other)
{
    if(this != &other)
    {
        double *tmp_data = new double[other.getM() * other.getN()];

        for(unsigned int i = 0; i < other.getM(); ++i)
        {
            for(unsigned int j = 0; j < other.getN(); ++j)
            {
                tmp_data[i * other.getN() + j] = *other[i * other.getN() + j];
            }
        }

        _m = other.getM();
        _n = other.getN();

        delete [] _data;
        _data = tmp_data;
    }

    return *this;
}

double* Matrix::operator[](unsigned const int& i) const
{
    return &_data[i * _n];
}

Matrix Matrix::operator*(const Matrix b) const
{
    Matrix result(this->getM(), b.getN());

    if(this->getN() != b.getM())
    {
        std::cerr << "Invalid Matrix size." << std::endl;
        throw std::logic_error("Invalid Matrix size.");
    }

    for(unsigned int i = 0; i < this->getM(); ++i)
    {
        for(unsigned int j = 0; j < b.getN(); ++j)
        {
            double value = 0.0;
            for(unsigned int index = 0; index < b.getM(); ++index)
            {
                value += (*this)[i][index] * b[index][j];
            }
            result[i][j] = value;
        }
    }

    return result;
}

void Matrix::print()
{
    for(unsigned int i = 0; i < _m; ++i)
    {
        for(unsigned int j = 0; j < _n; ++j)
        {
            std::cout << (*this)[i][j];
            if(j < _n - 1)
            {
                std::cout << " | ";
            }
        }
        std::cout << std::endl;
    }
}

Jetzt funktioniert es... Dachte ich ^^ solange, bis ich die print()-Funktion wieder genutzt habe. Seitdem stürzt das Programm wieder beim 3. oder 4. mal der Benutzung des Operators * ab. Wenn ich sie wieder aus meinem Code entferne gibt es "keine" Probleme mehr(bestimmt, aber man merkt es auf jeden Fall nicht einfach so). Woran könnte das liegen
Danke noch mal,
MfG
DragonRaider

Dein Kopierkonstruktor sieht eher wie eine Zuweisung aus. Du brauchst gar keinen temporären Puffer und das delete[] _data; ist tödlich.

Das Interface gefällt mir sehr viel besser, da hast Du aufmerksam gelesen.

C++ Forumbot

Dieser Thread wurde von Moderator/in SeppJ aus dem Forum C (C89, C99 und C11) in das Forum C++ (auch C++0x und C++11) verschoben.

Im Zweifelsfall bitte auch folgende Hinweise beachten:
C/C++ Forum :: FAQ - Sonstiges :: Wohin mit meiner Frage?

Dieses Posting wurde automatisch erzeugt.

Und wenn man statt Zeiger, new/delte/memset und "Regel der Drei" bzw. "Regel der Fünf" einfach std::vector nimmt, wird es viel einfacher.

DragonRaider

Hallo,
@Furble Wurble: Freut mich, dass das Ganze jetzt besser ist ich habe die Funktion print() auch noch const gemacht(verändert ja auch nichts) wegen dem Kopierkonstruktor: Da hast Du Recht, da hatte ich nicht richtig drüber nachgedacht. Das hat mein Problem gelöst vielen Dank besonders an Dich, aber auch an alle Anderen die mir geholfen haben
@manni66: Du hast Recht, so ginge es natürlich. Aber so habe ich doch viel mehr gelernt, oder? Jetzt weiß ich (wennn bestimmt auch noch nicht perfekt), wann man const bei Memberfunktionen verwenden sollte und wie ein Zuweisungsoperator und ein Kopierkonstruktor so ungefähr auszusehen haben. Und ich kenne mehr mögliche Fehlerquellen. Das alles wird mir in Zukunft helfen, eigene Klassen zu schreiben, die vielleicht nicht ganz so simpel sind/sich nicht "umgehen" lassen.
MfG
DragonRaider

PS.: Vielen Dank auch für's Verschieben

DragonRaider

Hi,
ich bin's noch mal. Ich war mir nicht sicher, ob das wirklich in nen anderen Thread sollte... Darum hier Das Problem ist jetzt nämlich: Wenn ich den Zuweisungsoperator ganz pur nach dem Motto

A = B;

verwende, stürzt das Programm ab ^^
MfG
DragonRaider

Marc-O

Hallo

Ich würde dir folgende Artikel nochmal ans Herz legen: Überladung von Operatoren in C++ (Teil 1). Gerade wegen deinem Zuweisungsoperator.

Wegen deinem direkten Problem, solltest du deinem Kopiert-Konstruktor und deine Zuweisungs-Operator debuggen um zu sehen wo genau der Fehler auftritt.

Edit: Die Code-Zeile "delete [] _data" hast du hoffentlich aus dem Kopiekonstruktor entfernt. Zum anderen hast du innerhalb deines Kopiert-Konstruktors sowie auch im Zuweisungsoperator fehlerhafte Indexzugriffe ins Array, hier ein angepasster Zuweisungsoperator:

Matrix& Matrix::operator=(Matrix const& other)
{
  if (this != &other)
  {
    double *tmp_data = new double[other.getM() * other.getN()];

    for (unsigned int i = 0; i < other.getM(); ++i)
      for (unsigned int j = 0; j < other.getN(); ++j)
        // i muss mit getN multipliziert werden
        tmp_data[i * other.getN() + j] = other[i][j]; 
        // Beim zugriff auf _other_ wird nicht direkt ins _data gegriffen, sondern auf deinen 
        // ueberladenen Operator, den du implementiert hast. Ansonst koenntest du es noch so machen:
        //tmp_data[i * other.getN() + j] = other._data[i * other._n + j];

    _m = other.getM();
    _n = other.getN();

    delete[] _data;
    _data = tmp_data;
  }

  return *this;
}

Mfg mdn

camper

tmp_data[i * other.getN() + j] = *other[i * other.getN() + j];

die rechte Seite sieht seltsam aus...

Ich finde den operator[] , der einen double* zurückgibt äusserst suspekt.

Der klassische Rat ist einen double& operator()(int x, int y); zu implementieren und auf operator[] zu verzichten.

Ansonsten ist es recht schwierig einen Fehler zu erkennen, wenn Du nicht die aktuelle Implementierung postest.

DragonRaider

Hallo,
ich habe mich bemüht, schon einmal auf eure Einwände ein zu gehen. Im Code ist deshalb auch einiges Kommentiert.
@Marc-O: _data ist private, also ist direkter zugriff nicht möglich(oder verwechsle ich da etwas?).

mat.h

#ifndef MATRIX_LIBRARY_H
#define MATRIX_LIBRARY_H

class Matrix
{
public:
    Matrix(unsigned int m, unsigned int n);                //m = rows, n = columns
    Matrix(const Matrix &other);
    ~Matrix();

    unsigned int getN() const;
    unsigned int getM() const;

    Matrix operator*(const Matrix b) const;
    double* operator[](unsigned const int& i) const;
    double& operator()(unsigned int i, unsigned int j);
    Matrix& operator=(const Matrix other);

    void print() const;

private:
    unsigned int _n, _m;
    double *_data;
};

#endif

mat.cpp

#include "mat.h"
#include <stdexcept>
#include <cstdlib>
#include <cstring>
#include <iostream>

Matrix::Matrix(unsigned int m, unsigned int n) : _n(n), _m(m)
{
    _data = new double[_m * _n];
    memset(_data, 0, sizeof(double) * _m * _n);
}

Matrix::Matrix(const Matrix& other)
{
    _data = new double[other.getM() * other.getN()];

    /*for(unsigned int i = 0; i < other.getM(); ++i)
    {
        for(unsigned int j = 0; j < other.getN(); ++j)
        {
            _data[i * other.getN() + j] = *other[i * other.getN() + j];
        }
    }*/
    for(unsigned int i = 0; i < other.getM() * other.getN(); ++i)
    {
        _data[i] = other[i][0];                                                                //Besser verständlich?
    }

    _m = other.getM();
    _n = other.getN();
}

Matrix::~Matrix()
{
    delete [] _data;
}

unsigned int Matrix::getN() const
{
    return _n;
}

unsigned int Matrix::getM() const
{
    return _m;
}

Matrix& Matrix::operator=(const Matrix other)
{
    if(this != &other)
    {
        double *tmp_data = new double[other.getM() * other.getN()];

        /*for(unsigned int i = 0; i < other.getM(); ++i)
        {
            for(unsigned int j = 0; j < other.getN(); ++j)
            {
                tmp_data[i * other.getN() + j] = *other[i * other.getN() + j];                   //Der Operator[] gibt die Addresse mit einem "Offset" zurück, sodass man die von **Arrays gewohnten Dinge machen kann.
            }                                                                                    //*other[i * other.getN() + j] ist somit das gleiche wie other[i * other.getN() + j][0]
        }*/
                                                                                                 //Jedoch ist diese Form sowieso ineffizient
        for(unsigned int i = 0; i < other.getM() * other.getN(); ++i)
        {
            tmp_data[i] = other[i][0];                                                           //Besser verständlich?
        }

        _m = other.getM();
        _n = other.getN();

        delete [] _data;
        _data = tmp_data;
    }

    return *this;
}

double* Matrix::operator[](unsigned const int& i) const                                          //Meiner Meinung nach intuitiver, da es sich verhält wie ein double** Array
{
    return &_data[i * _n];
}

double& Matrix::operator()(unsigned int i, unsigned int j)                                       //Auch gut ^^
{
    return _data[i * _n + j];
}

Matrix Matrix::operator*(const Matrix b) const
{
    Matrix result(this->getM(), b.getN());

    if(this->getN() != b.getM())
    {
        std::cerr << "Invalid Matrix size." << std::endl;
        throw std::logic_error("Invalid Matrix size.");
    }

    for(unsigned int i = 0; i < this->getM(); ++i)
    {
        for(unsigned int j = 0; j < b.getN(); ++j)
        {
            double value = 0.0;
            for(unsigned int index = 0; index < b.getM(); ++index)
            {
                value += (*this)[i][index] * b[index][j];
            }
            result[i][j] = value;
        }
    }

    return result;
}

void Matrix::print() const
{
    for(unsigned int i = 0; i < _m; ++i)
    {
        for(unsigned int j = 0; j < _n; ++j)
        {
            std::cout << (*this)[i][j];
            if(j < _n - 1)
            {
                std::cout << " | ";
            }
        }
        std::cout << std::endl;
    }
}

MfG
DragonRaider

Belli

Du kannst Dir übrigens in Deinem Kopierkonstruktor den Aufruf der get-Methoden sparen:

//_m = other.getM();
_m = other._m; //usw.

camper

_data[i] = other[i][0];                                                                //Besser verständlich?

leider immer noch falsch. Überlege nochmal genau, was du mit dem Argument i in deinem überladenen []-Operator anstellst.

camper

class Matrix
{
public:
    Matrix(unsigned int m, unsigned int n);                //m = rows, n = columns
// wenn m und n einer Erläuterung bedürfen, warum sie dann nicht gleich so benennen? also _rows, _columns
// - dann sieht man u.a. auch viel schneller bei Tippfehlern durch, m und n sind ja nicht gerade einfach optische beim Überfliegen auseinanderzuhalten
// - und einbuchstabige Bezeichner haben in einem Interface sowieso nichts verloren - die hebt man sich für sehr kleine Scopes auf
    Matrix(const Matrix &other);
    ~Matrix();

    unsigned int getN() const; // bin kein Fan von diesere Namenswahl für elementare Accesoren
    unsigned int getM() const;

    Matrix operator*(const Matrix b) const;               // der Parameter sollte eine Referenz sein
    double *operator[](unsigned const int& i) const;      // Matrix ist eine "Wert"-Klasse - Wenn die Matrix konstant ist, sollten es auch die Elemente sein
                                                          // der Rückgabetyp sollte somit const double* sein
                                                          // eine non-const-Überladung ist dann natürlich ebenso erforderlich
    Matrix& operator=(Matrix other);                      // siehe operator* (eine clevere Optimierung mit dieser Signatur existiert,
                                                          // aber besser ist es, erst mal die normale Technik zu lernen)

    void print();                                         // könnte wahrscheinlich const sein, soll ja sicher nicht die Matrix verändern

private:
    unsigned int _n, _m;
    double *_data;
};

Matrix::Matrix(unsigned int m, unsigned int n) : _n(n), _m(m)
{
    _data = new double[_m * _n];                          // dass kann auch in der Initialisierungsliste passieren und sollte es der Konsitenz wegen auch
    memset(_data, 0, sizeof(double) * _m * _n);           // memset ist hier nur bedingt geeignet, fill/fill_n angemessen und die sofortige Initialisierung
                                                          // bei der Allokation die beste Variante: new foo[...]() oder in C++11 auch new foo[...]{}
}

// Symmetrie ist zwar nicht zwingend erforderlich, dürfte aber das Vertrauen in die Korrektheit des Codes erhöhen
// Die Verwendung der Initialisierungsliste ist auch hier zu empfehlen
Matrix::Matrix(const Matrix& other)
{
    _data = new double[other.getM() * other.getN()];

    /*for(unsigned int i = 0; i < other.getM(); ++i)
    {
        for(unsigned int j = 0; j < other.getN(); ++j)
        {
            _data[i * other.getN() + j] = *other[i * other.getN() + j]; // falsch, da *other[i * other.getN() + j] zu other.data_[(i * other._n + j) * other._n] wird
                                                                        // Weil der fehlerhafte Zugriff beim Lesen stattfindet, ist es nicht unwahrscheinlich, dass das
                                                                        // Programm erst einmal läuft, nur mit fehlerhaften Daten.
                                                                        // Stünde der Unfug auf beiden Seiten, würde sich das Problem wahrscheinlich viel eher äußern,
                                                                        // und so die Fehlersuche beschleunigen.
// z.B:     _data[i * other.getN() + j] = other._data[i * other.getN() + j];
// oder     (*this)[i][j] = other[i][j];
        }
    }*/
    for(unsigned int i = 0; i < other.getM() * other.getN(); ++i)
    {
//         _data[i] = other[i][0];                                                                //Besser verständlich?  - gleiches Problem, s.o.
// z.B.    _data[i] = other.data_[i];
// oder    (*this)[0][i] = other[0][i];
    }
 // Es gibt nat. auch noch passende Algorithmen in der Standardbibliothek
    _m = other.getM();
    _n = other.getN();
}

Matrix::~Matrix()
{
    delete [] _data;
}

unsigned int Matrix::getN() const
{
    return _n;
}

unsigned int Matrix::getM() const
{
    return _m;
}

Matrix& Matrix::operator=(Matrix other)
{
    if(this != &other)    // das ist sowieso immer erfüllt, wenn der other-Parameter keine Referenz ist - clever ist es bei dieser Signatur einfach den Inhalt von other zu stehlen,
                          // denn das ist sowieso schon eine temporäre Kopie des Argumentes, die nicht mehr gebraucht wird, ganz nebenbei erspart man sich den Code, der fast
                          // identisch zu dem im Konstruktor ist, nochmal zu schreiben (ggf. mit den gleichen oder anderen Fehlern).
    {
        double *tmp_data = new double[other.getM() * other.getN()]; // schön: erst neue Resourcen anfordern, bevor alte entsorgt werden
                                                                    // und damit wird der Test this != &other erst recht überflüssig

        for(unsigned int i = 0; i < other.getM(); ++i)
        {
            for(unsigned int j = 0; j < other.getN(); ++j)
            {
                tmp_data[i * other.getN() + j] = *other[i * other.getN() + j]; // siehe Kopiekonstruktor
            }
        }

        _m = other.getM();
        _n = other.getN();

        delete [] _data;
        _data = tmp_data;
    }

    return *this;
}

double* Matrix::operator[](unsigned const int& i) const
{
    return &_data[i * _n];
}

Matrix Matrix::operator*(const Matrix b) const
{
    Matrix result(this->getM(), b.getN());

    if(this->getN() != b.getM())                             // sollte zweckmäßigerweise vor der Erzeugung von result kommen (Kleinigkeit)
    {
        std::cerr << "Invalid Matrix size." << std::endl;
        throw std::logic_error("Invalid Matrix size.");
    }

    for(unsigned int i = 0; i < this->getM(); ++i)
    {
        for(unsigned int j = 0; j < b.getN(); ++j)
        {
            double value = 0.0;
            for(unsigned int index = 0; index < b.getM(); ++index)
            {
                value += (*this)[i][index] * b[index][j];
            }
            result[i][j] = value;
        }
    }

    return result;
}

DragonRaider

Hi,
ich überarbeite gerade den Code - Vielen Dank In der Zeile

... = other[I * other.getN() + j];

hatte ich wirklich nicht richtig nachgedacht. Müssen die Argumente bei Operatoren eigentlich immer Referenzen sein? Oder bringt das irgendwelche Vorteile(mal abgesehen davon, dass man sie u.U. verändern will)?
Das Überprüfen des Pointers hatte ich aus dem Wikipediaartikel zur Rule of Three. Erst hatte ich im Zuweisungsoperator std::copy genutzt, was nicht funktioniert hatte... Jedoch liegt das am gleiche Problem wie das oben -> mit other._data würde es funktionieren? Probiere ich gleich mal, der geupdatet Code kommt auch bald.
Die "Kürzel/Begriffe/Was-auch-immer" m und n sind für Matrizen so festgelegt, jedoch wäre es tatsächlich wahrscheinlich leichter anstattdessen rows und cols o.ä. zu verwenden. Melde mich gleich noch mal, wenn der geupdatete Code fertig ist
MfG
DragonRaider

Edit: Ich verstehe den Sinn der const Überladung des Operators[] noch nicht so ganz, da dieser ja auch zur Bearbeitung der Matrix benutzt werden soll. Aber jetzt wo ich so drüber nachdenke... Bewirkt das dann einfach, dass ich den Wert des Pointers nicht ändern kann (ziemlich dumme Frage, aber da bin ich mir gerade wirklich nicht sicher)

DragonRaider

Hallo,
jetzt mal die aktuelle Version. Ich hoffe ich habe keinen Deiner Vorschläge für Verbesserungen vergessen. Letztendlich habe ich mich doch für die Namen m und n entschieden, da ich diese dadurch auswendig lerne, was in der Mathematik nicht schaden kann ^^

mat.h:

#ifndef MATRIX_LIBRARY_H
#define MATRIX_LIBRARY_H

class Matrix
{
public:
    Matrix(unsigned int m, unsigned int n);                //m = rows, n = cols
    Matrix(const Matrix &other);
    ~Matrix();

    unsigned int getN() const;
    unsigned int getM() const;

    Matrix operator*(const Matrix& b) const;
    double* operator[](unsigned const int& i) const;
    double& operator()(unsigned const int& i, unsigned const int& j) const;
    Matrix& operator=(const Matrix& other);

    void print() const;

private:
    unsigned int _n, _m;
    double *_data;
};

#endif

mat.cpp:

#include "mat.h"
#include <stdexcept>
#include <cstdlib>
#include <cstring>
#include <iostream>

#define DEBUG                                    //Sollte immer definiert sein wenn Anwendung nicht zu 100% sicher
#define DEBUGLEVEL_2                             //Etwas langsamer, aber leichter zu debuggen

Matrix::Matrix(unsigned int m, unsigned int n) : _n(n), _m(m), _data(new double[n * m])
{
    std::fill(_data, &_data[n * m], 0.0);
}

Matrix::Matrix(const Matrix& other) : _n(other._n), _m(other._m), _data(new double[other.getM() * other.getN()])
{
    std::copy(other._data, &other._data[other._n * other._m - 1], _data);
}

Matrix::~Matrix()
{
    delete [] _data;
}

unsigned int Matrix::getN() const
{
    return _n;
}

unsigned int Matrix::getM() const
{
    return _m;
}

Matrix& Matrix::operator=(const Matrix& other)
{
    double *tmp_data = new double[other._m * other._n];

    std::copy(other._data, &other._data[other._m * other._n], tmp_data);

    _m = other._m;
    _n = other._n;

    delete [] _data;
    _data = tmp_data;

    return *this;
}

double* Matrix::operator[](unsigned const int& i) const                                          //Meiner Meinung nach intuitiver, da es sich verhält wie ein double** Array
{
    #ifdef DEBUG
    if(i >= _m)
    {
        std::cerr << "Invalid index in operator[]." << std::endl;
        throw std::logic_error("Invalid index in operator[].");
    }
    #endif // DEBUG

    return &_data[i * _n];
}

double& Matrix::operator()(unsigned const int& i, unsigned const int& j) const                    //Auch gut ^^
{
    #ifdef DEBUG
    #ifdef DEBUGLEVEL_2
    if(i >= _m)
    {
        std::cerr << "Invalid index i in operator[]." << std::endl;
        throw std::logic_error("Invalid index i in operator[].");
    }
    if(j >= _n)
    {
        std::cerr << "Invalid index j in operator()." << std::endl;
        throw std::logic_error("Invalid index j in operator[].");
    }
    #else
    if(i >= _m || j >= _n)
    {
        std::cerr << "Invalid index in operator()." << std::endl;
        throw std::logic_error("Invalid index in operator().");
    }
    #endif
    #endif // DEBUG

    return _data[i * _n + j];
}

Matrix Matrix::operator*(const Matrix& b) const
{
    #ifdef DEBUG
    if(this->_n != b._m)
    {
        std::cerr << "Invalid Matrix size." << std::endl;
        throw std::logic_error("Invalid Matrix size.");
    }
    #endif

    Matrix result(this->_m, b._n);

    for(unsigned int i = 0; i < this->_m; ++i)
    {
        for(unsigned int j = 0; j < b._n; ++j)
        {
            double value = 0.0;
            for(unsigned int index = 0; index < b._m; ++index)
            {
                value += (*this)[i][index] * b[index][j];
            }
            result[i][j] = value;
        }
    }

    return result;
}

void Matrix::print() const
{
    for(unsigned int i = 0; i < _m; ++i)
    {
        for(unsigned int j = 0; j < _n; ++j)
        {
            std::cout << (*this)[i][j];
            if(j < _n - 1)
            {
                std::cout << " | ";
            }
        }
        std::cout << std::endl;
    }
}

MfG
DragonRaider

camper

DragonRaider schrieb:

Edit: Ich verstehe den Sinn der const Überladung des Operators[] noch nicht so ganz, da dieser ja auch zur Bearbeitung der Matrix benutzt werden soll. Aber jetzt wo ich so drüber nachdenke... Bewirkt das dann einfach, dass ich den Wert des Pointers nicht ändern kann (ziemlich dumme Frage, aber da bin ich mir gerade wirklich nicht sicher)

Gegenwärtig kann ich schreiben

const Matrix x(1,1);
x[0][0] = 42;

Wenn aber x konstant ist, ist es doch unlogisch, wenn ich trotzdem den Inhalt verändern kann.

Also muss die Überladung des []-Operators unterscheiden, ob sie für ein konstantes Objekt aufgerufen wird oder nicht. Analog dann für den ()-Operator.

Nebenbei bemerkt, ist es in der Regel nicht sinnvoll, arithmetische Typen wie int per Referenz zu übergeben.
Ein assert ist bei Bereichsüberschreitungen in der Regel viel nützlicher als eine Exception.

const double* Matrix::operator[](unsigned i) const // für konstante Matizen
{
   assert(i < _m && "invalid index");
   return _data + i * _n;
}
double* Matrix::operator[](unsigned i) // für modifizierbare Matrizen
{
   return const_cast<double*>( (*const_cast<const Matrix*>(this))[i] ); // vermeidet Codeduplikation
}

const double& Matrix::operator()(unsigned i, unsigned j) const
{
   assert(j < _n && "invalid index");
   return (*this)[i][j];
}
double* Matrix::operator[](unsigned i)
{
   return const_cast<double*>( (*const_cast<const Matrix*>(this))(i,j) );
}