3. Gründe für malloc NULL zu returnen

Gründe für malloc NULL zu returnen

  • D

    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

    0
  • M

    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

    0
  • C
    Mod 

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

    die rechte Seite sieht seltsam aus...

    0
  • ?

    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.

    0
  • D

    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

    0
  • B

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

    //_m = other.getM();
_m = other._m; //usw.
    0
  • C
    Mod 

    _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.

    0
  • C
    Mod 

    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;
}
    0
  • D

    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)

    0
  • D

    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

    0
  • C
    Mod

    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) );
}
    0
Anmelden zum Antworten