Problem bei selbstgeschriebener Matrixklasse

Hallo!
Ich wollte aus reiner Übung eine Klasse schreiben, die mit (2D)Matritzen rechnen kann. Da ich den Umgang mit Arrays und Pointern üben möchte, bitte nicht "Benutz doch vector!!" schreien

Meine Klasse sieht wie folgt aus:

class Matrix
{
   private:
    size_t X;
    size_t Y;
    double** pointer;

   public:
    Matrix();
    Matrix(size_t Y, size_t X);  //Anzahl Zeilen, Anzahl Spalten
    ~Matrix();

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

    void ausgeben()
    {
        for(size_t y=0;y!=Y;++y)
        {
            for(size_t x=0;x!=X;++x)
                std::cout<<pointer[x][y]<<" ";
            std::cout<<std::endl;
        }
    }     
};

Die Konstruktoren schauen wie folgt aus:

Matrix::Matrix() : X(0), Y(0)
{
    pointer=NULL;
}

Matrix::Matrix(size_t spalten, size_t zeilen) : X(zeilen), Y(spalten)
{
    pointer = new double*[X]; //zeiger auf array aus zeigern
    for(size_t i=0;i!=X;++i)
        pointer[i] = new double[Y]; //verknüpft jeden zeiger des arrays mit einem array
}

Jetzt habe ich zwei Operatoren überladen. Zum einen den += Operator und den + Operator. Der += Operator funktioniert auch wunderbar

Matrix& Matrix::operator+=(Matrix const& obj)
{
    if(X != obj.X || Y != obj.Y)
        std::cout<<"FEHLER: Spalten/Zeilen nicht identisch!!"<<std::endl;
    else
    {
        for(size_t y=0;y!=Y;++y)
            for(size_t x=0;x!=X;++x)
                pointer[x][y]+=obj.pointer[x][y];
    }
    return (*this);
}

Wenn ich allerdings mit dem + Operator rechnen will, stürzt mein Programm ab. Hier die Überladung des + Operators:

Matrix Matrix::operator + (const Matrix& obj) const
{
    Matrix tmp(*this);

    if(tmp.X != obj.X || tmp.Y != obj.Y)
        std::cout<<"Spalten/Zeilen nicht identisch!!"<<std::endl;
    else
    {
        for(size_t y=0;y!=tmp.Y;++y)
            for(size_t x=0;x!=tmp.X;++x)
                tmp.pointer[x][y]+=obj.pointer[x][y];
    }

    return tmp;
}

Wenn ich diese Aktionen durchführe passiert folgendes:

int main()
{
    Matrix a(3,3);   //
    Matrix b(3,3);   // Objekte anlegen
    Matrix Ergebnis; //
    a.set(); //werte eingeben, hier liegt der Fehler nicht
    b.set();
    a+=b;
    a.ausgeben() //FUNKTIONIERT

    Ergebnis=a+b;
    Ergebnis.ausgeben(); //Absturz
}

Das Programm läuft so lange stabil, bis Ergebnis ausgegeben wird. Ich kann sogar auf einzelne Elemente von Ergebnis zugreifen, ohne dass etwas unvorhergesehenes passiert. Wenn aber .ausgeben() ausgeführt wird, stürzt mein Programm ab. Hat jemand eine Idee, woran das liegen könnte bzw. wie ich herausfinden könnte, woran das liegt?

P.S.: Über Tipps zu Stil / Optimierungen freue ich mich auch

Xebov

Also ich denke mal ich sehe deinen Fehler. Also erstmal dir fehlt ein Copykonstruktor und ein operator=, ohne die wird das Standartverhalten durchgeführt und das Bedeutet das die variablen nur einander zugewiesen werden. Im falle deines operator+ bedeutet das das deine Zeile

Matrix tmp(*this);

das einfach alles aus this in tmp kopiert wird. Nun verfügt deine Klasse aber über einen Zeiger. Das Standartverhalten holt aber keinen Speicher sondern kopiert den Zeiegr einfach, dh der Zeiegr aus tmp und aus this zeigen auf den selben Speicherbereich. Nun gibst du das tmp objekt zurück, es wird dann ins Ergebniss kopiert, dh das in diesem Moment bereits 3 instanzen auf den selben Speicherbereich zeigen. Nach dem kopieren stirbt tmp weil es nichtmehr gebraucht wird, da du aber zweifellos im Destruktor den Zeiger freigibst gibt tmp den zeiger auch frei, da Ergebniss und a aber den selben Bereich auch nutzen wird ihnen der Speicher unter den Füßen weggenommen.

Ich hoffe das war verständlich, also bastel dir noch nen Copykonstruktor und nen operator= und dann passt das.

Im übrigen kannst du dir auch den Doppelten Code im operator+ sparen indem du nach dem kopieren von this in tmp einfach tmp+=obj machst, das ruft den +0operator von tmp auf und erspart dir den doppelten Code.

Vielen Dank! Genau daran lag es.
Habe den Code einfach um die Zeilen

Matrix& Matrix::operator = (const Matrix& obj)
{
    X=obj.X;
    Y=obj.Y;
    pointer = new double*[X]; //zeiger auf array aus zeigern
    for(size_t i=0;i!=X;++i)
        pointer[i] = new double[Y];

    for(size_t y=0;y!=Y;++y)
        for(size_t x=0;x!=X;++x)
            pointer[x][y]=obj.pointer[x][y];
    return (*this);
}

Matrix::Matrix(const Matrix& obj)
{
    X=obj.X;
    Y=obj.Y;

    pointer = new double*[X]; //zeiger auf array aus zeigern
    for(size_t i=0;i!=X;++i)
        pointer[i] = new double[Y];

    for(size_t y=0;y!=Y;++y)
        for(size_t x=0;x!=X;++x)
            pointer[x][y]=obj.pointer[x][y];
}

ergänzt und jetzt funktioniert alles einwandfrei.
Ist der Kopierkonstruktor und der Zuweisungsoperator auch stilistisch so OK?
Und macht es eigentlich Sinn, im Konstruktor Matrix(Zeilen, Spalten) alle Werte der Matrix über zwei For-Schleifen auf 0.0 zu setzen?

Xebov

Mtrx schrieb:

Ist der Kopierkonstruktor und der Zuweisungsoperator auch stilistisch so OK?
Und macht es eigentlich Sinn, im Konstruktor Matrix(Zeilen, Spalten) alle Werte der Matrix über zwei For-Schleifen auf 0.0 zu setzen?

Stilistisch OK ja. Nen Tip hätte ich aber noch, du kannst das ganze auch so schreiben

Matrix& Matrix::operator = (const Matrix& obj)
{
    X=obj.X;
    Y=obj.Y;
    pointer = new double*[X]; //zeiger auf array aus zeigern
    for(size_t i=0;i!=X;++i)
        pointer[i] = new double[Y];

    for(size_t y=0;y!=Y;++y)
        for(size_t x=0;x!=X;++x)
            pointer[x][y]=obj.pointer[x][y];
    return (*this);
}

Matrix::Matrix(const Matrix& obj)
{
    this->operator=(obj);
}

Damit ersparst du dir doppelten Code weil beide Funktionen genau das selbe tun.

Was das vorbelegen mit 0en angeht, ob soetwas sinn macht hängt immer von der Nutzung ab. Wenn man einen Vorteil hat weil 0en drinstehen dann kann man es tun, wenn man aber sowieso imemr die Matrix mit gültigen Werten füllt bevor man sie benutzt dann braucht man es nicht tun, sowa shängt immer vom vorhaben ab.

pumuckl

Mtrx schrieb:

Und macht es eigentlich Sinn, im Konstruktor Matrix(Zeilen, Spalten) alle Werte der Matrix über zwei For-Schleifen auf 0.0 zu setzen?

Ja, macht es durchaus. Damit hast du dann eine Initialisierung vorgenommen und der Konstruktor liefert ein definiertes Objekt, mit dem die Benutzer sofort arbeiten können. Andernfalls hättest du immer irgendwelchen Mist drinstehen direkt nach der Konstruktion und der Benutzer wäre gezwungen, die Initialsierung aller Elemente selber vorzunehmen.

Grundsätzlich würde ich mir aber Gedanken über das Design deiner Matrixklasse machen: Bei dir ist es theoretisch möglich, zwei Matrizen verschiedener Dimensionen zusammenzuaddieren.

Ich würde das vermutlich so lösen, dass ich die Dimensionen als zwei Templateparameter einbauen würde.

eine abschließende Frage habe ich noch:
Wenn ich zB. zwei Matrix Objekte erstelle und beide mit werten fülle

Matrix a(3,3), b(3,3);
a.set();
b.set();

und dann b=a; umkopiere, müsste ich dann im = Operator nicht zuerst das 2D array in b löschen? denn wenn ich den zeiger von b mit dem zeiger/den werten von a initialisiere, belegt der alte zeiger doch noch speicherplatz (weil kein Destruktor aufgerufen wurde); oder wird der Speicherplatz des Arrays automatisch freigegeben, wenn ich den Zeiger auf einen anderen Wert zeigen lasse?

Werner Salomon

Mtrx schrieb:

... und jetzt funktioniert alles einwandfrei.

Nein nicht alles, Du hast im Zuweisungsoperator versäumt einen eventuell allokierten Speicher (Member pointer usw.) wieder freizugeben. Damit schaffst Du Memoryleaks, wenn die 'alte' Matrix vorher nicht leer war.

Xebov schrieb:

Matrix::Matrix(const Matrix& obj)
{
    this->operator=(obj);
}
Damit ersparst du dir doppelten Code weil beide Funktionen genau das selbe tun.

Das Vermeiden von redundantem Code ist löblich. Üblich ist hier aber die umgekehrte Methode, nämlich den Kopiekonstruktor zu implementieren und diesen im Zuweisungsoperator zu nutzen. Dafür ist dann noch ein swap notwendig und man landet beim Copy-and-Swap. Damit wird es auch exception-sicher, wenn der Kopiekonstruktor exception-sicher ist.

Gruß
Werner

Xebov

Mtrx schrieb:

und dann b=a; umkopiere, müsste ich dann im = Operator nicht zuerst das 2D array in b löschen? denn wenn ich den zeiger von b mit dem zeiger/den werten von a initialisiere, belegt der alte zeiger doch noch speicherplatz (weil kein Destruktor aufgerufen wurde); oder wird der Speicherplatz des Arrays automatisch freigegeben, wenn ich den Zeiger auf einen anderen Wert zeigen lasse?

Ja musst du du musst Dynamische Sachen erst aufräumen und dann neu zuweisen. Automatisch gelöscht werden mit new angelegte Objekte nicht. Würdest du dem Zeiger einfach einen neuen Bereich zuweisen würde der alte als Speicherleck bestehen bleiben.

Werner Salomon schrieb:

Das Vermeiden von redundantem Code ist löblich. Üblich ist hier aber die umgekehrte Methode, nämlich den Kopiekonstruktor zu implementieren und diesen im Zuweisungsoperator zu nutzen. Dafür ist dann noch ein swap notwendig und man landet beim Copy-and-Swap. Damit wird es auch exception-sicher, wenn der Kopiekonstruktor exception-sicher ist.

Ok das ist gut zu wissen, gleichmal anschaun. Wobei es ja auch umgedreht gilt, wenn der oeprator Exception sicher ist dann würde ja auch der Konstruktor sicher werden oder?

pumuckl

Xebov schrieb:

Ok das ist gut zu wissen, gleichmal anschaun. Wobei es ja auch umgedreht gilt, wenn der oeprator Exception sicher ist dann würde ja auch der Konstruktor sicher werden oder?

Nur wenn die Konstruktoren der Member, die ja implizit auch aufgerufen werden, auch exceptionsicher sind.