Probleme bei Return by Reference oder was anderem



  • Hi !!!

    Ich habe folgenden Code:
    Datei Matrix.h

    #define index(i,j) (i*n+j)
    class Matrix
    {
    public:
      Matrix(int M = 0, int N = 0, int Z = 0);
      Matrix(const Matrix& m) : n(0) { *this=m; }
      ~Matrix(); 
    
      double       *operator[] (int i)       { return a+i*n; }
      const double *operator[] (int i) const { return a+i*n; }
      double& operator() (int i, int j)       { return a[index(i,j)]; }
      double  operator() (int i, int j) const { return a[index(i,j)]; }
      const Matrix& operator= (const Matrix& m);
      Matrix& transpose(const Matrix& A);
    
    private:
      int m, n, z; //Dimension und Anzahlt der Elemente ungleich 0
      double *a;		
    };
    

    Datei Matrix.cpp

    Matrix::Matrix(int M, int N, int Z): m(M), n(N), z(Z)
    { 
      if ((n != 0) && m!= 0 )
        a=new double[m*n];
    
      for(int i = 0; i < m; i++)
        for( int j = 0; j < n; j++)
          a[index(i,j)] = 0;
    }
    
    Matrix::~Matrix()
    { 
      if (n || m) 
        delete[] a; 
    } 
    
    const Matrix& Matrix::operator= (const Matrix& A)
    {
      if (m || n) delete[] a; 
    
      m = A.m; 
      n = A.n;
      a=new double[m*n]; 
      copy(A.begin(),A.end(),a);
    
      return *this;
    }
    
    Matrix& Matrix::transpose()
    {
      Matrix result(*this);
    
      for( int i = 0; i < m; i++ )
        for( int j = 0; j < n; j++ )
           result(i,j)=a[index(j,i)];
    
      return result;
    }
    

    Datei main:

    #include "matrih.h"
    int main(int argc, char *argv[])
    {
      Matrix A(3,3);
      A[2][0] = -4;
      Matrix B(3,3);
    
      B = A.transpose();  
    }
    

    Das Programm wird problemlos kompiliert, doch während der Ausführung bekomme ich einen Semgentation Fault.
    Liegt es daran, dass mein transpose eine Referenz zurück gibt und die Matrix result dann nicht mehr existiert bei der Zuweisung = ?
    Was müsste ich denn ändern, damit das trotzdem klappt, wenn die Referenz zurück gegeben wird?
    Es sollte schon eine Referenz zurück gegeben werden, da die Matrizen mit denen ich arbeite sehr sehr gross sind, so im Bereich 10000x10000. Ah ja, die Matrix A sollte nicht verändert werden.
    Da ich leider noch keine allzu grosse Erfahrung im Programmieren habe, und das vor kurzem gelernte im Programm anwenden möchte, fehlen mir die Ideen, wie ich das umsetzen kann.
    Ich dachte schon an Zeiger und new Operator, doch wäre das die Lösung, und wie geht dann di eAnwendung von new in einer selbst erstellten Klasse? Weil die Deklaration in transpose
    result = new Matrix(3,3);
    leider nicht funktioniert (error: result undeclared)

    Ich wäre für Hilfe sehr dankbar, da ich momentan nicht weiter weiss.

    EDIT: Den Code soweit erweitert, dass er den Copykonstruktor enthält.



  • Hallo,

    Du gibst bei transpose eine Referenz auf ein lokales Objekt zurück. Das funktioniert natürlich nicht. Du mußt hier eine Kopie machen. Damit das geht brauchst du hier lediglich einen Copyconstructor für Matrix.



  • Ok, ich habe es soweit verändert.
    Doch es funktiert immer noch nicht.
    Nun gibt es schwirigkeiten mit den Pointern 😞
    *** glibc detected *** free(): invalid pointer: 0xb7ed2ff4 ***



  • Zeig doch mal deine Änderungen.



  • Der Zuweisungsoperator müsste auf jeden Fall noch gegen Selbstzuweisung geschützt werden



  • oder anders designen (swap). Dafür brauchts dann natürlich einen (funktionierenden) CopyCtor.



  • Matrix(const Matrix& m) : n(0) { *this=m; }
    

    Hier erhält m und a einen zufälligen Wert. Damit wird der Zuweisungsoperator aufgerufen.

    const Matrix& Matrix::operator= (const Matrix& A)
    {
      if (m || n) delete[] a;
    

    Wenn m einen zufälligen Wert ungleich 0 hat, wird delete[] auf diesen zufälligen Pointer a aufgerufen, was mit ziemlicher Sicherheit fehlschlägt. Wahrscheinlich hast Du die Abrfage "m && n" gemeint.

    Du solltest den Pointer a immer mit 0 initialisieren. Dann ist eine delete[] darauf erlaubt und Du kannst diese Abfragen "if (m || n)" weg lassen.

    Tommi


  • Mod

    CStoll schrieb:

    Der Zuweisungsoperator müsste auf jeden Fall noch gegen Selbstzuweisung geschützt werden

    Braunstein schrieb:

    oder anders designen (swap). Dafür brauchts dann natürlich einen (funktionierenden) CopyCtor.

    oder einfach nur alles in der richtigen reihenfolge tun:
    1. neuen speicher anfordern
    2. kopieren
    3. alten speicher löschen
    ein copy-zuweisungsoperator, der gegen selbstzuweisung geschützt werden muss (nichts gegen eine optimierung, wenn sie hilft), ist im grunde immer ein defekt. denn auch mit dem test gegen selbstzuweisung ist der operator sonst nicht exception-safe. ein copyconstruktor, der das objekt nur halbfertig konstruiert und dann den zuweisungsoperator aufruft ist auch unheilbar.



  • Braunstein schrieb:

    Zeig doch mal deine Änderungen.

    Ich habe den Copykonstruktor hinzugefügt und in der Funktion transpose:
    Matrix result( int M, int N, int Z );
    durch
    Matrix result(*this);
    ersetzt.

    CStoll schrieb:

    Der Zuweisungsoperator müsste auf jeden Fall noch gegen Selbstzuweisung geschützt werden

    Wie kann ich das machen?

    Braunstein schrieb:

    oder anders designen (swap). Dafür brauchts dann natürlich einen (funktionierenden) CopyCtor.

    Hmm, ist mein Copy Konstruktor falsch? Wie müsste ich ihn richtig machen?



  • Du solltest die transpose()-Methode auf jeden Fall so ändern, daß sie einen Wert zurückgibt.

    Und zur Selbstzuweisung: Das Problem ist, daß du bei einer Zuweisung "A=A;" (die sich durchaus irgendwo verstecken könnte) den Matrix-Inhalt zerstörst, den du später kopieren willst. Als Lösung mußt du entweder abfragen, ob du dich selber kopieren willst oder erst kopieren und dann die alten Daten löschen (@camper: den Ansatz sehe ich auch als eine Variante des Zuweisungsschutzes ;)):

    //Variante 1:
    Matrix& operator=(const Matrix& other)
    {
      if(this!=*other)
      {
        //kopieren
      }
      return *this;
    }
    
    //Variante 2:
    Matrix& operator=(const Matrix& other)
    {
      double* tmp = new double[other.m*other.n];
      m=other.m;
      n=other.n;
      for(int i=0;i<m*n;++i) a[i]=other.a[i];
      delete[]a;a=tmp;
      return *this;
    }
    

  • Mod

    bucada schrieb:

    Braunstein schrieb:

    Zeig doch mal deine Änderungen.

    Ich habe den Copykonstruktor hinzugefügt und in der Funktion transpose:
    Matrix result( int M, int N, int Z );
    durch
    Matrix result(*this);
    ersetzt.

    CStoll schrieb:

    Der Zuweisungsoperator müsste auf jeden Fall noch gegen Selbstzuweisung geschützt werden

    Wie kann ich das machen?

    Braunstein schrieb:

    oder anders designen (swap). Dafür brauchts dann natürlich einen (funktionierenden) CopyCtor.

    Hmm, ist mein Copy Konstruktor falsch? Wie müsste ich ihn richtig machen?

    hier noch mal die drei möglichkeiten:
    1. mit schutz gegen selbstzuweisung (nicht exception-sicher)

    Matrix::Matrix(const Matrix& rhs)
        : m( rhs.m ), n( rhs.n ), z( rhs.z ), a( new double[ rhs.m * rhs.n ] )
    {
        std::copy( rhs.a, rhs.a + rhs.m * rhs.n, a );
    }
    Matrix& Matrix::operator=(const Matrix& rhs)
    {
        if ( this != &rhs )
        {
            delete [] a;
            m = rhs.m;
            n = rhs.n;
            z = rhs.z;
            a = new double[ rhs.m * rhs.n ];
            std::copy( rhs.a, rhs.a + rhs.m * rhs.n, a );
        }
        return *this;
    }
    

    oder mit richtiger reihenfolge (ohne test und exceptionsicher, copyctor bleibt wie oben):

    Matrix& Matric::operator=(const Matrix& rhs)
    {
        double* tmp = new double[ rhs.m * rhs.n ];
        std::copy( rhs.a, rhs.a + rhs.m * rhs.n, tmp );
        delete [] a;
        m = rhs.m;
        n = rhs.n;
        z = rhs.z;
        a = tmp;
        return *this;
    }
    

    oder mit copy&swap (ebenfalls ohne test und exceptionsicher):

    Matrix& Matric::operator=(const Matrix& rhs)
    {
        Matrix( rhs ).swap( *this );
        return *this;
    }
    void Matrix::swap(Matrix& lhs, Matrix& rhs)
    {
        std::swap( lhs.m, rhs.m );
        std::swap( lhs.n, rhs.n );
        std::swap( lhs.z, rhs.z );
        std::swap( lhs.a, rhs.a );
    }
    

  • Mod

    CStoll schrieb:

    @camper: den Ansatz sehe ich auch als eine Variante des Zuweisungsschutzes 😉

    das problem ist eigentlich nicht selbstzuweisung sondern exceptionsicherheit. tatsache ist, dass ein (stark, weniger ist für copy-operationen kaum vertretbar) exceptionsicherer operator automatisch auch sicher für selbstzuweisung ist.



  • CStoll schrieb:

    Du solltest die transpose()-Methode auf jeden Fall so ändern, daß sie einen Wert zurückgibt.

    Und zur Selbstzuweisung: Das Problem ist, daß du bei einer Zuweisung "A=A;" (die sich durchaus irgendwo verstecken könnte) den Matrix-Inhalt zerstörst, den du später kopieren willst.

    Oh ja, daran habe ich gar nicht gedacht 😮

    Das mit dem Wert zurückgeben ist aber ein Problem, weil wie gesagt die Matrizen mit denen ich rumrechne riesig sind.
    Nach der Änderung des Zuweisungsoperators habe ich folgendes Problem.
    Die Transponierung findet statt.
    Eine Ausgabe vor dem return gibt mir die richtige Matrix.
    Der operator= funktioniert auch.
    Es wird nur was falsches zurückgegeben.
    Die Matrix die rauskommt ist eine 134537360 x 134533128 Matrix, wo die ersten Elemente diejenigen sind, die rauskommen sollen.
    Sie wird nicht ganz ausgegeben, weil dann ein Segmentation Fault kommt 😞


  • Mod

    beim transponieren erhälst du aus einer m*n matrix eine n*m matrix. die kann auf keinen fall direkt durch kopieren entstehen, offensichtlich wenn n!=m ist. und auch für m==n müsstest du ein swap einsetzen der aus der originalmatrix kopieren, sonst geht dir die hälfte verloren. etwa so:

    Matrix Matrix::transpose()
    {
        Matrix result( n, m, z );
        for( int i = 0; i < m; i++ )
            for( int j = 0; j < n; j++ )
               result(j,i)=(*this)(i,j);
        return result;
    }
    

    falls das erstellen von kopien zu teuer ist, z.b. weil die matrizen riesig sind, könnte man über lazy evaluation nachdenken. etwa durch expression templates oder dessen runtime equivalent (dekorator).



  • Ok, es funktioniert mitlerweile. 🙂
    Ich habe es mit Hilfe von Zeigern gemacht.

    Matrix* Matrix::transpose()
    {
      Matrix* result;
      result = new Matrix((*this));
    
      for( int i = 0; i < result->m; i++ )
        for( int j = 0; j < result->n; j++ )
           (*result)(i,j) = a[index(j,i)];
    
      return result;
    }
    

    Ich musste dann zwar meine main Datei entsprechend anpassen, aber es funktioniert jetzt.


  • Mod

    bucada schrieb:

    Ok, es funktioniert mitlerweile. 🙂
    Ich habe es mit Hilfe von Zeigern gemacht.

    Matrix* Matrix::transpose()
    {
      Matrix* result;
      result = new Matrix((*this));
      
      for( int i = 0; i < result->m; i++ )
        for( int j = 0; j < result->n; j++ )
           (*result)(i,j) = a[index(j,i)];
      
      return result;
    }
    

    Ich musste dann zwar meine main Datei entsprechend anpassen, aber es funktioniert jetzt.

    das funktioniert nicht und die verwendung von zeigern bringt hier keinen vorteil:
    einfaches besipiel, transponiere eine 1*2 matrix:
    die zuweisungen sind:
    result(0,0)=m(0,0)
    result(0,1)=m(1,0)
    da allerdings n in beiden objekten gleich (nähmlich 2) ist, ergibt das
    result.a[0*2+0]=m.a[0*2+0]
    result.a[0*2+1]=m.a[1*2+0] und wir greifen auf ein element von m zu, das dieses gar nicht hat. nächster halt: segmentation fault.



  • Natürlich. Du hast recht.
    Es muss heissen:

    result = new Explicit(n, m, 0);
    

    bei der Initialisierung der Matrix result.
    Danke für den Hinweis. Bei mir funktionierte es, weil ich es mit quadratischen Matrizen ausprobiert habe.
    Ich sollte meine Beispiele demnächst besser wählen 😉


Log in to reply