3. delete verhält sich in main anders als in member function

delete verhält sich in main anders als in member function

  • J

    Hallo,
    ich dachte ja immer ich kenne mich mit C++ ein wenig aus... Aber mein aktuelles Problem treibt mich noch in den Wahnsinn.

    Ich habe das Problem auf drei Zeilen runtergekocht. Ich habe eine minimalistische Klasse geschrieben, die ich zum schreiben von Matrizen in 1d C-Arrays nutze: Matrix, bzw davon abgeleitet FMatrix. Diese Klasse möchte ich in einer anderen Klasse verwenden. Dort habe ich nun folgenden (längst nicht mehr sinnvollen) Code:

    FMatrix<double>* matrix  = NULL;                                                           
    matrix = new FMatrix<double>(2,3);                                                        
    delete matrix;

    delete sorgt nun dafür, dass der Destruktor von FMatrix aufgerufen wird, doch dann gibt es den Fehler

    Speicherzugriffsfehler (Speicherabzug geschrieben)

    Der Witz ist, dass dies nur in der Klassenfunktion passiert. In einem losgelösten Testprogramm funktioniert das freigeben problemlos. Sprich ich habe eine main Methode mit genau den selben code Zeilen und die funktioniert.
    Ich denke, dass man Probleme mit Namensräumen ausschließen kann, da ich den Variablennamen schon so geändert habe, dass er definitiv einzig artig ist.

    Das folgende ist der Code der Matrizen:

    #ifndef _ARRAY_HXX
#define _ARRAY_HXX
#include<cstring>
#include <iostream>
template<typename T> class Matrix{
    public:
        T* data;
        int w,d;
        Matrix():data(NULL),w(0),d(0){}// width is in x direction and  depth in y 
        Matrix(double width,double depth){
            data=new T[(int)width*(int)depth];
            w=(int)width;
            d=(int)depth;
        }   
        virtual ~Matrix(){
            std::cout << "destructor" << std::endl;
            delete[] data;
        }   
        virtual T& operator()(int x,int y)=0;
};

template<typename T> class FMatrix:public Matrix<T>{
    public:
        FMatrix(int width,int depth):Matrix<T>(width,depth){
        }   
        ~FMatrix(){
            std::cout << " fmat destructor" << std::endl;
        }
        T& operator()(int x, int y){
            return this->data[y*this->w+x];
        }
};

    Den Code, wo ich die oben gezeigten Zeilen einbinde, habe ich nicht angehängt, da dieser ein wenig umfangreicher ist.

    Nun zum Schluss noch einmal die Frage, die mich am stärksten um treibt:
    Wie kann die Bedeutung des Code-Schnippsels von dem Ort des Aufrufs abhängen?

    Wäre genial, wenn mir jemand weiterhelfen könnte! Vielen Dank!

    0
  • S

    Wenn ich das ganze ein wenig umstelle, funktioniert alles wunderbar (auch wenn der Code ziemlich komisch ist - wieso double , wozu nicht operator[] statt operator() , usw.):

    #include <iostream>

template<typename T>
class Matrix
{
protected:
      T* data;
      int w,d;

public:
        Matrix():
        data(NULL),
        w(0),
        d(0)
        {}// width is in x direction and  depth in y

        Matrix(double width,double depth):
        data(new T[width * depth]),
        w(width),
        d(depth)
        {}

        virtual ~Matrix()
        {
            std::cout << "Destructor\n";
            if(data)//Die Abfrage hier ist sehr wichtig! Sonst versuchst du nicht allokierten Speicher freizugeben (was selbstverständlich zu einem Laufzeitfehler führt)!
                  delete[] data;
        }

        virtual T& operator()(int x, int y) = 0;
};

template<typename T>
class FMatrix : public Matrix<T>
{
    public:
        FMatrix(int width,int depth):
        Matrix<T>(width,depth){}

        ~FMatrix()
        {
            std::cout << "FMatrix destructor\n";
        }

        T& operator()(int x, int y)
        {
            return Matrix<T>::data[y * Matrix<T>::w + x];
        }
};

int main()
{
      FMatrix<double> a(5.9, 4.7);
}

    Achtung: Mit deinem Code darfst du nicht den Default-Ctor verwenden! Sonst gibt es (s. o.) einen Laufzeitfehler... am besten einfach gar nicht deklarieren...

    0
  • J

    Hi,
    vielen Dank Hacker für die schnelle Antwort.

    Ich sehe ein, dass vieles nicht mehr so sinnvoll ist. Das meiste ist meiner Fehlersuche zum Opfer gefallen.

    Beide Versionen ( meine und deine angepasste ) lassen sich in einem kleinen Testprogramm deklarieren, initialisieren und auch wieder (mit delete) freigeben.
    (ich musste bei deiner den Konstruktor von Matrix zu (int,int) machen...)

    Soweit so gut. Was mich in den Wahnsinn treibt ist, dass es in einer Memberfunction einer anderen Klasse anders aussieht. Auch mit deiner veränderten Version. Wie ist das möglich?

    0
  • S

    J.Bourne schrieb:

    Soweit so gut. Was mich in den Wahnsinn treibt ist, dass es in einer Memberfunction einer anderen Klasse anders aussieht. Auch mit deiner veränderten Version. Wie ist das möglich?

    Zeig mal Code.

    0
  • S

    J.Bourne schrieb:

    Soweit so gut. Was mich in den Wahnsinn treibt ist, dass es in einer Memberfunction einer anderen Klasse anders aussieht. Auch mit deiner veränderten Version. Wie ist das möglich?

    Du ueberschreibst irgendwo Speicher. Und dann explodiert es an einer anderen Stelle (zB im Destruktor eines Objektes das eigentlich OK ist)

    0
  • J

    Wie schon erwähnt, das wird jetzt etwas länger. Allerdings sollte sich das Problem in einem kleinen Teil finden:
    In main wird halt das erste mal eine Matrix erzeugt(initMatrix) und wenn dann writeTestSetting aufgerufen wird, wird die Dimension angepasst und dabei eine neue Matrix allociert. Hier scheitert das delete vor dem allokieren.

    Bzw. so wie ich es jetzt hochgeladen habe deklariere ich in initMatrix eine ganz neue Variable (s. ursprüglicher Codeschnipsel) und trotzdem geht es schief....

    Hier kommt die Basisklasse

    #include<cmath>
#include<iostream>
#include<string>
#include"linsolvers.hpp"
#include"array.hxx"
#include<gsl/gsl_vector.h>

using namespace std;

//create symbolic system with natural numbers as valid grid points
//this is a simple but VERY expensive way of finding neighbors
void LinSolvers::createSymbolicSystem(){
    delete cube;
    cube = new Cube<int>(n+2,n+2,n+2);
    for ( int i  = 0;i<n+2;i++)
        for ( int j  = 0;j<n+2;j++)
            for ( int k  = 0;k<n+2;k++)
                (*cube)(i,j,k)=0;
    int count = 1;
    for ( int i  = 1;i<n+1;i++)
        for ( int j  = 1;j<n+1;j++)
            for ( int k  = 1;k<n+1;k++){
                (*cube)(i,j,k)=count;
                count++;
            }
}

LinSolvers::LinSolvers(int dims, int n):ticktack(false){
    this->matrix = NULL;
    this->solution = NULL;
    this->rhs = NULL;
    this->dims = dims;
    this->n = n;
    this->N = pow(n,dims);
    this->cube = NULL;
    createSymbolicSystem();
}

void LinSolvers::printMatrix(void){
    printSystem(false);
}
void LinSolvers::printSystem(void){
    printSystem(true);
}
void LinSolvers::printSystem(bool withRhs){
    for (int i = 0; i < N; i++){
        for (int j = 0; j < N; j++){
            cout << getElement(i, j) << "  ";
        }
        if (withRhs)
            cout << "   " << rhs[i];
        cout << endl;
    }
}
//c storage scheme
void LinSolvers::getFullIndex(int l, int &i, int &j, int &k){
    i=l/(this->n*this->n);
    j=(l%(this->n*this->n))/this->n;
    k=l%this->n;
}
// test if this index is valid for cube
bool LinSolvers::isValidIndex(int i,int j,int k){
    if ((i<0)||(i>=n+2)||(j<0)||(j>=n+2)||(k<0)||(k>=n+2))
        return false;
    else
        return true;
}
bool LinSolvers::isValidNode(int i, int j){
    int i1,j1,k1,i2,j2,k2;
    getFullIndex(i, i1,j1,k1);
    i1++;//cube starts at 1
    j1++;
    k1++;
    if (!isValidIndex(i1,j1,k1))
        return false;
    getFullIndex(j, i2,j2,k2);
    i2++;//cube starts at 1
    j2++;
    k2++;
    for (int ii=-1; ii<2;ii++)
        for (int jj=-1; jj<2;jj++)
            for (int kk=-1; kk<2;kk++){
                if ((ii!=0)&&(jj!=0)&&(kk!=0)) // This is because the kernel contains 19 elements instead of 27
                    continue;
                if (!isValidIndex(i2+ii,j2+jj,k2+kk))
                    continue;
                if (((*cube)(i1,j1,k1)==(*cube)(i2+ii,j2+jj,k2+kk))&&((*cube)(i1,j1,k1)!=0)){
                    return true;
                }
            }
    return false;
}

int LinSolvers::getLinIndex(int i, int j, int k){
    return i*n*n+j*n+k;
}
void LinSolvers::tryToAddToElement(int i, int j, double value){
    if (isValidNode(i,j))
        addToElement(i,j,value);
}

// G spatially independent 
void LinSolvers::writeTestSetting(int type, int rhsNumber){
    bool nameSet = false;
char names[][80] = {
        "../documentation/data/diag_0.375.dat",
        "../documentation/data/diag_0.3775.dat",
        "../documentation/data/diag_0.625.dat",
        "../documentation/data/diag_0.6275.dat",
        "../documentation/data/diag_0.75.dat",
        "../documentation/data/diag_0.755.dat",
        "../documentation/data/spiral_2.1025.dat",
        "../documentation/data/spiral_2.1.dat",
        "../documentation/data/spiral_2.1025.dat",
        "../documentation/data/spiral_2.7025.dat",
        "../documentation/data/spiral_2.7.dat",
        "../documentation/data/spiral_4.5.dat",
        "../documentation/data/spiral_4.5025.dat"};

    switch (type){
    case 0: {
                // create simple system
                this->setdims(1);
                this->setn(3);
                int b[3] = {-1,3,-3};
                for (int i = 0; i < N; i++){
                    //cout << i << "  " <<  endl;
                    setElement(i, b[i]);
                }
                double A[3][3] = {{3,1,3},{1,2,3},{2,6,5}};
                for (int i = 0; i < N; i++){
                    for (int j = 0; j < N; j++){
                        //cout << i << "  " << j << endl;
                        setElement(i,j,A[i][j]); 
                    }
                }

                break;
            }
    case 1: {
                this->setdims(2);
                this->setn(50);
                double G[3][3] = {{1,0,0},{0,1,0},{0,0,1}}; // This is G for normal laplace
                writeDiscreteOperator(G);
                loadRhsFromFile(names[rhsNumber]);
                break;
            }
    case 2: {
                this->setdims(2);
                this->setn(50);
                double G[3][3] = {{7./6.,1./6.,2./3.},{1./6.,5./3.,1./6.},{2./3.,1./6.,7./6.}}; // This is G for D_parallel = 2 und D_perpendicular=1 and a fiber orientation f= (1,1,1) (normed).
                writeDiscreteOperator(G);
                loadRhsFromFile(names[rhsNumber]);
                break;
            }
    }
}
void LinSolvers::loadRhsFromFile( char fileName[]){
    string *name = new std::string(fileName);
    gsl_vector *rhs=gsl_vector_alloc(N);
    FILE * file = fopen((*name).c_str(),"r");
    gsl_vector_fread(file,rhs);
    for (int i = 0; i < N; i++){
        setElement(i, gsl_vector_get(rhs, i));
    }
}

void LinSolvers::writeDiscreteOperator(double G[3][3]){
    for (int i = 0; i < N; i++){
        tryToAddToElement(i,i,-4*G[0][0]-4*G[1][1]-4*G[2][2]);//no shift
        for (int j = 0; j < 3; j++){
            int c=1; // collapse integer
            if (dims == j)// if there are just 2 dimensions: write to origin
                c = 0;
            tryToAddToElement(i,i-c*pow(n,-j+2),2*G[j][j]);//single shift ; the strange power has its origin in the storage structure of c
            tryToAddToElement(i,i+c*pow(n,-j+2),2*G[j][j]);
        }
        for (int ii = 0; ii < 3; ii++){//double shift
            for (int jj = 0; jj < 3; jj++){
                if ( ii == jj )
                    continue;
                int c; // collapsed index
                if ((dims == ii)||(dims == jj))// if there are just 2 dimensions: write to origin
                    c = 0;
                for (int iiv = -1 ; iiv <2; iiv+=2){//sign of shift
                    for (int jjv = -1 ; jjv <2; jjv+=2){//sign of shift
                        tryToAddToElement(i,i+iiv*c*pow(n,-ii+2),iiv*jjv*0.5*G[ii][jj]);
                        tryToAddToElement(i,i+jjv*c*pow(n,-jj+2),iiv*jjv*0.5*G[ii][jj]);
                        tryToAddToElement(i,i+iiv*c*pow(n,-ii+2)+jjv*c*pow(n,-jj+2),iiv*jjv*0.5*G[ii][jj]);
                    }
                }
            }
        }
    }

}

void LinSolvers::setdims(int dims){
    if (this->dims == dims)
        return;
    this->dims=dims;
    this->N = pow(n,dims);
    initMatrix();
    initRhs();
    createSymbolicSystem();
}
void LinSolvers::setn(int n){
    if (this->n == n)
        return;
    this->n = n;
    this->N = pow(n,dims);
    initMatrix();
    initRhs();
    createSymbolicSystem();
}

    Header:

    #ifndef LINSOLVERS_HPP
#define LINSOLVERS_HPP
#define SYSSIZE 4
#define DIMS 2
#include<string>
#include<cstring>
#include"array.hxx"
#include"ticktack.h"
class LinSolvers{
    public: 
        virtual void setElement(int i, int j, double value)=0;
        virtual void setElement(int i, double value)=0;
        virtual void addToElement(int i, int j, double value)=0;
        virtual double getElement(int i, int j)=0;
        virtual double getElement(int i)=0;
        void writeTestSetting(int , int =0);
        void printMatrix(void);
        bool isValidNode(int,int);
        void tryToAddToElement(int,int,double);
        virtual void initMatrix(void)=0;
        virtual void initRhs(void)=0;
        LinSolvers(int dims=DIMS, int n=SYSSIZE);
        virtual int solve()=0; 
        virtual void printSolution()=0;
        void printSystem(void);
        void printSystem(bool);
        void writeDiscreteOperator(double G[3][3]);
    protected:
        virtual void setn(int n);
        virtual void setdims(int dims);
        int dims;
        int N;
        int n;
        Matrix<double>* matrix;
        double *rhs;
        double *solution;
        Cube<int> *cube;
        TickTack ticktack;
    private:
        void loadRhsFromFile( char fileName[]);
        void getFullIndex(int l, int &i, int &j, int &k);
        bool isValidIndex(int i,int j,int k);
        void createSymbolicSystem();
        int getLinIndex(int i, int j, int k);
};

#endif

    Davon erbt:

    #include"lapacksolverbanded.hpp"
#include<cstring>
#include<cmath>

using namespace std;

void LapackSolverBanded::setElement(int i, double value){
    rhs[i] = value;
}
double LapackSolverBanded::getElement(int i){
    return rhs[i];
}
void LapackSolverBanded::initRhs(){
    cout << "start delete in rhs" << endl;
    delete[] rhs;
    delete[] solution;
    cout << "end delete in rhs" << endl;
    this->rhs = new double[N];
    this->solution = new double[N];
    for (int i = 0; i < N; i++){
        this->setElement(i,0.0);
    }
}
void LapackSolverBanded::initMatrix(){
    FMatrix<double>* atrix = NULL;
    atrix = new FMatrix<double>(2,3);
    delete atrix;
    cout << "geschrieben " << endl;
    cout << "second delete done" << endl;

}

// this function will ignore calls with parameters that are out of bound
void LapackSolverBanded::setElement(int i, int j, double value){
    if (value == 0.0)
        return;
    if ( (i<0)||(i>=N)||(ndiags/2<abs(i-j)) )
        return;
    (*matrix)(ndiags-1+i-j,j) = value;
}
// this function will ignore calls with parameters that are out of bound
void LapackSolverBanded::addToElement(int i, int j, double value){
    if (value == 0.0)
        return;
    if ( (i<0)||(i>=N)||(ndiags/2<abs(i-j))||(j>=N) )
        return;
    (*matrix)(ndiags-1+i-j,j) = value;
}

// this function will return 0. to calls with parameters that are out of bound
double LapackSolverBanded::getElement(int i, int j){
    if ( (i<0)||(i>=N)||(ndiags/2<abs(i-j))||(j>=N) )
        return 0.;
    return  (*matrix)(ndiags-1+i-j,j);
}

LapackSolverBanded::LapackSolverBanded(){
    ndiags = 2*n*n*(dims/3)+2*n*(dims/2)+3;
}

extern "C" {
void dgbsv_(int *,int *,int *,int *,double *,int *,int*,double *,int*,int*);
};

int LapackSolverBanded::solve(){
    // Values needed for dgesv
    int nrhs = 1;
    int ipiv[N];
    int info;
    int ldab = (ndiags+ndiags/2);
    int ku =  ndiags/2;

    cout << "ku: " << ku << endl;
    cout << "ldab: " << ldab << endl;
    cout << "N: " << N << endl;
    cout << "ndiags: " << ndiags << endl;
    for (int i = 0; i < N*ldab; i++){
        cout << matrix->data[i] << "  ";
    }

    // Solve the linear system
    cout << "START" << endl;
    ticktack.timespan = 0;
    ticktack.tick();
    dgbsv_(&N,&ku , &ku, &nrhs, matrix->data, &ldab, ipiv, rhs, &N, &info);
    ticktack.tack();
    cout << "END" << endl;
    cout << "Time elapsed: " << ticktack.timespan << endl;
    for (int i = 0; i < N; i++)
        solution[i] = rhs[i];
    return info;
}

void LapackSolverBanded::printSolution(){
    cout << "Solution: ";
    for (int i = 0; i < N; i++)
        cout << solution[i] << " ";
    cout << endl;
}

void LapackSolverBanded::setn(int n){
    ndiags = 2*n*n*(dims/3)+2*n*(dims/2)+3;
    LinSolvers::setn(n);
}

void LapackSolverBanded::setdims(int dims){
    ndiags = 2*n*n*(dims/3)+2*n*(dims/2)+3;
    LinSolvers::setdims(dims);
}

    Header:

    #ifndef LAPACKSOLVERBANDED_HPP
#define LAPACKSOLVERBANDED_HPP
#include"linsolvers.hpp"

class LapackSolverBanded : public LinSolvers{
    public:
        void initMatrix();
        LapackSolverBanded();
        void setElement(int,int,double);
        void addToElement(int, int, double);
        double getElement(int i,int j);
        void setElement(int i, double value);
        double getElement(int i);
        void initRhs();
        int solve();
        void printSolution();
    protected:
        void setn(int n);
        void setdims(int dims);
    private:
        int ndiags;
};
#endif

    main:

    #include"lapacksolver.hpp"
#include"lapacksolverbanded.hpp"
#include<iostream>

using namespace std;
int main(void){
    LapackSolverBanded solver;
    cout << "initMatrix" << endl;
    solver.initMatrix();
    cout << "initRhs" << endl;
    solver.initRhs();
    cout << "write" << endl;
    solver.writeTestSetting(0);
    cout << "print" << endl;
    solver.printSystem();
    cout << "solve" << endl;
    cout <<  solver.solve() << endl;
    solver.printSolution();
/*
 *
    //cout << "writeSetting" << endl;
    for (int i = 1; i < 3; i++){
        for (int j = 0; j < 8; j++){
            solver.writeTestSetting(i,j);
            cout <<  solver.solve() << endl;
        }
    }
    cout << "Start REVERSE" << endl;
    for (int i = 2; i > 0; i--){
        for (int j = 7; j >= 0; j--){
            solver.writeTestSetting(i,j);
            cout <<  solver.solve() << endl;
        }
    }
    */
}
    0
  • O

    Auch wnen wir dir sagen: "zeig mal Code"

    solltest du immer noch den Sticky oben beachten:

    http://www.c-plusplus.net/forum/304133

    reduzier deinen Code soweit, das zwar noch der Fehler auftritt, aber alles unnötige entfernt wurde.

    0
  • S

    Ich sag dir gleich eine Sache: Versuche so wenig Speiche wie möglich selbst zu allokieren. Wieso nicht gleich Stackobjekte?

    Und wozu überhaupt

    FMatrix<double>* atrix = NULL;
    atrix = new FMatrix<double>(2,3);
    delete atrix;

    ? Du nutzt atrix doch nicht einmal... 😕

    0
  • O

    ich befürchte, hacker, dass das testcode ist um den Fehler zu provozieren.

    0
  • ?

    Dein code ist prinzipiell unglaublich unübersichtlich. Wenn du das Problem angehst, ist es wohl auch leichter den Fehler zu finden - oder du behebst ihn "versehentlich".

    1.Viel zu viel rum-gezeigere.

    string *name = new std::string(fileName); //warum mit new?

    Matrix<double>* matrix;
double *rhs;
double *solution;
Cube<int> *cube;

    Warum sind das alles Zeiger? Warum keine STL container?

    2. Speicherlecks. Destruktor, copy-ctor und assignment-operator fehlen völlig. Früher oder später explodiert dir das also sowieso.

    3. Du initialisierst die Klasse nicht richtig im konstruktor. Deswegen hast du vielleicht so viele Zeiger, um "nicht initialisiert" zu kennzeichnen. Das ist aber schon ein grundsätzliches Problem! Ist die Modellierung vielleicht Fehlerhaft?

    bool LinSolvers::isValidNode(int i, int j){
    int i1,j1,k1,i2,j2,k2;
    getFullIndex(i, i1,j1,k1);
    i1++;//cube starts at 1
    j1++;
    k1++;
    if (!isValidIndex(i1,j1,k1))
        return false;
    getFullIndex(j, i2,j2,k2);
    i2++;//cube starts at 1
    j2++;
    k2++;
    for (int ii=-1; ii<2;ii++)
        for (int jj=-1; jj<2;jj++)
            for (int kk=-1; kk<2;kk++){
                if ((ii!=0)&&(jj!=0)&&(kk!=0)) // This is because the kernel contains 19 elements instead of 27
                    continue;
                if (!isValidIndex(i2+ii,j2+jj,k2+kk))
                    continue;
                if (((*cube)(i1,j1,k1)==(*cube)(i2+ii,j2+jj,k2+kk))&&((*cube)(i1,j1,k1)!=0)){
                    return true;
                }
            }
    return false;
}

    Wenn cube nicht zufälligerweise in void LinSolvers::createSymbolicSystem() erstellt wurde, crasht es hier direkt - wenn du Glück hast. Das Problem hängt eng mit 1. und 3. zusammen

    void LapackSolverBanded::setElement(int i, double value){
    rhs[i] = value;
}
double LapackSolverBanded::getElement(int i){
    return rhs[i];
}

    Woher weißt du, ob es bei rhs an der Stelle i überhaupt ein Element gibt? Warum erstellst du rhs nicht im Kontruktor?

    double A[3][3] = {{3,1,3},{1,2,3},{2,6,5}};
for (int i = 0; i < N; i++){
    for (int j = 0; j < N; j++){
    //cout << i << "  " << j << endl;
    setElement(i,j,A[i][j]);
}

    Was für einen Wert hat N? Da das mit unbekannten konstanten erstellt wird, weiß ich das nicht - aber falls du diese konstanten änderst, hast du hier auf jedenfall einen bug - wenn du Glück hast einen crash, wenn du Pech hast seltsame Fehler, die nur manchmal auftreten!

    So, ich hör mal auf, aber dein Problem ist sehr sehr grundlegend.
    Zwing dich mal dazu, deinen code so zu schreiben, das du:
    a) Keine Init funktionen hast.
    b) Alle Werte direkt im ctor so initialisiert werden, dass du damit arbeiten kannst.
    c) Dich im weiteren Verlauf immer auf diese Werte beziehst. Keine magic numbers.
    d) Du destruktor, copy-ctor und Zuweisungsoperator korrekt schreibst.

    Das dürfte eine ziemlich lange Arbeit werden, aber dann hast du evtl chancen dass du auch langfristig funktionierenden code haben könntest.

    0
  • ?

    Grad gesehen, das N wohl 16 sein dürfte. Das dürfte im aktuellem Fall dein Problem sein. Ändert aber nichts an deinem grundsätzlichem Designproblem.

    0
  • J

    Hallo Leute,
    vielen Dank euch allen und KMT im Besonderen!

    Zum Fehler: Wie bereits prophezeit, ist er bei der recht umständlichen Eingrenzung verschwunden. Eine Zeit lang konnte ich ihn rekonstruieren, aber nicht festnageln. Zur Zeit glaube ich, dass es gar kein "Quelltext-Fehler" war, sondern dass er daher rührte, dass ich ein Objektfile einer anderen Klasse mit gelinkt habe(versehentlich->makefile). Zumindest konnte ich den Fehler durch das Entfernen aller Objektdateien beheben.

    Ich finde es großartig, dass ihr mir von euch aus so viele wertvolle Hinweise gegeben habt. Ich schreibe zwar in letzter Zeit immer mehr Code, aber es gibt eigentlich niemanden, der mich auf solche strukturellen Dinge hinweisen würde.

    KMT schrieb:

    1.Viel zu viel rum-gezeigere.

    string *name = new std::string(fileName); //warum mit new?

    Matrix<double>* matrix;
double *rhs;
double *solution;
Cube<int> *cube;

    Warum sind das alles Zeiger? Warum keine STL container?

    Zu 1.
    Der string-Pointer ist ein Relikt.
    solution und rhs sind pointer, da diese auf C-Arrays zeigen, die später einer Library-Funktion übergeben werden sollen, die nur solche akzeptiert.
    matrix und cube als pointer, damit ich dynamisch andere Instanzen erzeugen und zuweisen kann (neue Dimension oder Größe). Wäre es besserer Stil keine Pointer zu benutzen und der Klasse die Fähigkeit zu geben Größe etc. zu ändern?

    KMT schrieb:

    2. Speicherlecks. Destruktor, copy-ctor und assignment-operator fehlen völlig. Früher oder später explodiert dir das also sowieso.

    Zu 2.
    Destruktoren fehlen sicherlich. Ich habe bisher keinen Bedarf für copy-ctor und assignment-operator gesehen. Sollte man sie trotzdem immer initalisieren?

    KMT schrieb:

    3. Du initialisierst die Klasse nicht richtig im konstruktor. Deswegen hast du vielleicht so viele Zeiger, um "nicht initialisiert" zu kennzeichnen. Das ist aber schon ein grundsätzliches Problem! Ist die Modellierung vielleicht Fehlerhaft?

    Zu 3.
    Nun ich initialisiere alles, was beim Erzeugen des Objektes bekannt ist... Wenn dies mehr sein sollte, ist es vermutlich echt ein Modellproblem.

    KMT schrieb:

    bool LinSolvers::isValidNode(int i, int j){
    int i1,j1,k1,i2,j2,k2;
    getFullIndex(i, i1,j1,k1);
    i1++;//cube starts at 1
    j1++;
    k1++;
    if (!isValidIndex(i1,j1,k1))
        return false;
    getFullIndex(j, i2,j2,k2);
    i2++;//cube starts at 1
    j2++;
    k2++;
    for (int ii=-1; ii<2;ii++)
        for (int jj=-1; jj<2;jj++)
            for (int kk=-1; kk<2;kk++){
                if ((ii!=0)&&(jj!=0)&&(kk!=0)) // This is because the kernel contains 19 elements instead of 27
                    continue;
                if (!isValidIndex(i2+ii,j2+jj,k2+kk))
                    continue;
                if (((*cube)(i1,j1,k1)==(*cube)(i2+ii,j2+jj,k2+kk))&&((*cube)(i1,j1,k1)!=0)){
                    return true;
                }
            }
    return false;
}

    Wenn cube nicht zufälligerweise in void LinSolvers::createSymbolicSystem() erstellt wurde, crasht es hier direkt - wenn du Glück hast. Das Problem hängt eng mit 1. und 3. zusammen

    Zu 4.
    Ich bin mir nicht sicher, ob ich das richtig verstehe...
    Geht es darum, dass es eher eine Methode ist, die zu cube gehört? Und darum, dass sie eine Struktur von cube voraussetzt, die nicht vorgeschrieben ist?

    KMT schrieb:

    void LapackSolverBanded::setElement(int i, double value){
    rhs[i] = value;
}
double LapackSolverBanded::getElement(int i){
    return rhs[i];
}

    Woher weißt du, ob es bei rhs an der Stelle i überhaupt ein Element gibt? Warum erstellst du rhs nicht im Kontruktor?

    Zu 5.
    Fehlender Kontrollmechanismus->ok
    Ich initialisiere nicht, weil ich mehrfach verschiedene Daten in rhs schreiben will.

    KMT schrieb:

    double A[3][3] = {{3,1,3},{1,2,3},{2,6,5}};
for (int i = 0; i < N; i++){
    for (int j = 0; j < N; j++){
    //cout << i << "  " << j << endl;
    setElement(i,j,A[i][j]);
}

    Was für einen Wert hat N? Da das mit unbekannten konstanten erstellt wird, weiß ich das nicht - aber falls du diese konstanten änderst, hast du hier auf jedenfall einen bug - wenn du Glück hast einen crash, wenn du Pech hast seltsame Fehler, die nur manchmal auftreten!

    Zu 6.
    Ja, das ist nicht konsistent.

    KMT, um deine abschließenden Ratschläge erfüllen zu können wird mir wohl nichts anderes übrig bleiben, als das Ganze neu zu strukturieren und zu modellieren.

    Vielen Dank noch mal

    0
  • ?

    solution und rhs sind pointer, da diese auf C-Arrays zeigen, die später einer Library-Funktion übergeben werden sollen, die nur solche akzeptiert.
    matrix und cube als pointer, damit ich dynamisch andere Instanzen erzeugen und zuweisen kann (neue Dimension oder Größe). Wäre es besserer Stil keine Pointer zu benutzen und der Klasse die Fähigkeit zu geben Größe etc. zu ändern?

    Für arrays mit variabler Größe gibt es vector. Der hat den großen Vorteil, das er dir Speicherverwaltung abnimmt, und du so nicht so leicht Fehler mit der Speicherverwaltung machen kannst. Du kannst einen vector auch jederzeit als array übergeben, falls eine Funktion ein array erwartet. (vector.data() )
    Selbst wenn du irgendwelche Anforderungen hast, die vector (oder ein anderer container der STL) nicht erfüllt, schreib dir eine eigene kleine Klasse, die sich um die Verwaltung kümmert. So muss sich die Klasse, die die eigentliche Arbeit macht nicht um diese Verwaltungsaufgaben kümmern, und wird deutlich übersichtlicher.

    Destruktoren fehlen sicherlich. Ich habe bisher keinen Bedarf für copy-ctor und assignment-operator gesehen. Sollte man sie trotzdem immer initalisieren?

    Deklarier sie zumindest private. Wenn du dann ausversehen eine Kopie irgendwo erzeugst, bekommst du zumindest direkt einen compilerfehler, statt dich auf die Suche nach irgendwelchen Speicherzugriffsfehlern zu machen. Wenn du Zeiger verwendest erzeugen die vom compiler automatisch erzeugten eigentlich immer für Fehler.

    Nun ich initialisiere alles, was beim Erzeugen des Objektes bekannt ist... Wenn dies mehr sein sollte, ist es vermutlich echt ein Modellproblem.

    LapackSolverBanded solver;
    cout << "initMatrix" << endl;
    solver.initMatrix();
    cout << "initRhs" << endl;
    solver.initRhs();

    Hm, wenn du initMatrix() / initRhs() aufrufst, ist doch gar nicht so viel neues bekannt. Könntest du doch also direkt im Konstruktor aufrufen. 😉
    Falls es tatsächlich einmal sein sollte, das etwas nicht bekannt ist, wenn du ein Objekt erstellen willst, heißt es normalerweise, dass du zu früh versuchst, das Objekt zu erstellen. Erst alles zusammensuchen was man braucht, dann erstellen und verwenden.

    Ich bin mir nicht sicher, ob ich das richtig verstehe...
    Geht es darum, dass es eher eine Methode ist, die zu cube gehört? Und darum, dass sie eine Struktur von cube voraussetzt, die nicht vorgeschrieben ist?

    Mir fällt aber auf, das du createSymbolicSystem() im Konstruktor aufrufst. Passt also eigentlich alles, sorry hab ich übersehen. Sonst hätte es passieren können, das du "cube" mit "*cube" dereferenzierst, obwohl es noch 0 wäre.

    Mal eine Idee für den Anfang: Schreib dir mal eine Klasse für rhs.
    Also Konstruktor nimmst du die Funktion

    void loadRhsFromFile( char fileName[]);

    Sprich, du übergibst den Dateinamen, und dann hast du ein gültiges RHS. Zu testzwecken kannst du noch einen zweiten Konstruktor schreiben, dem man die Daten direkt übergeben kann. Da passen dann auch deine Funktionen getElement und setElement rein, eventuell auch andere Funktionen (ich schau mal nicht alles durch). Als nächstes kannst du dir überlegen, ob rhs wirklich ein member von deinem solver sein soll, und nicht lieber ein Parameter, den man übergibt, wenn er gebraucht wird. Versuch den Ansatz "Ich speicher erstmal alle Daten in einer Klasse, und rechne dann damit" zu vermeiden. Daten erstellen ist eine Aufgabe, die gar nichts mit deinem Algorithmus zu tun hat.

    0

  • @J.Bourne
    Implementier' den Zuweisungsoperator für deine Matrix-Klasse, dann kannst du die auch jederzeit ändern.
    BTW: C++ implementiert den Zuweisungsoperator auch automatisch für dich, aber in diesem Fall keinen passenden.

    Oder gleich noch besser: ändere deine Matrix Klasse so dass sie std::vector verwendet, statt selbst mit new[] und delete[] rumzumachen.
    Dann funktioniert nämlich auf einmal der compiler-generierte Zuweisungsoperator.

    0
  • J

    Vielen Dank noch einmal!
    Das ist wirklich hilfreich. Ich werde es dann mal versuchen in die Tat um zu setzen.
    Viele Grüße

    0
  • P

    J.Bourne schrieb:

    Zu 1.
    Der string-Pointer ist ein Relikt.

    Der war auch als er noch kein Relikt war ziemlich sicher schon überflüssig. Strings benutzt man so gut wie immer solo, nicht über Pointer.

    solution und rhs sind pointer, da diese auf C-Arrays zeigen, die später einer Library-Funktion übergeben werden sollen, die nur solche akzeptiert.

    Trotzdem vector benutzen, gibt ja vector::data()

    Wäre es besserer Stil keine Pointer zu benutzen und der Klasse die Fähigkeit zu geben Größe etc. zu ändern?

    Meist ja.

    [quote="KMT"]Zu 2.
    Destruktoren fehlen sicherlich. Ich habe bisher keinen Bedarf für copy-ctor und assignment-operator gesehen. Sollte man sie trotzdem immer initalisieren?
    [quote="KMT"] Wenn du einen Konstruktor schreibst, der mehr macht als nur Werte zu setzen (also wie hier z.B. Speicher allokiert), musst du fast zwangsläufig einen Dtor und einen Copy-Ctor und op= definieren. Falls du sie nicht brauchst, deklariere sie nur, und zwar als private, damit der compiler sie dir nicht "aus versehen" definiert ohne dass du merkst dass da Mist passiert.

    Ich initialisiere nicht, weil ich mehrfach verschiedene Daten in rhs schreiben will.

    Ist kein Grund, rhs nicht zu initalisieren 😉

    J.Bourne schrieb:

    Zur Zeit glaube ich, dass es gar kein "Quelltext-Fehler" war, sondern dass er daher rührte, dass ich ein Objektfile einer anderen Klasse mit gelinkt habe(versehentlich->makefile). Zumindest konnte ich den Fehler durch das Entfernen aller Objektdateien beheben.

    Das kann nur dann die Ursache gewesen sein, wenn du in mehreren Objektdateien unterschiedliche Definitionen gleichnamiger inline-Funktionen oder gleichnamiger Templates hattest. Dann kann allerdings Blödsinn bei rumkommen. Andernfalls haben die weggefallenen Objektdateien einfach nur die Auswirkungen des undefinierten Verhaltens geändert.

    0
Anmelden zum Antworten