Rätselhafter Konstruktor, bitte um Hilfe

chni

Hallo! Ich habe ein ganz seltsames Problem mit C++. Vielleicht kann mir jemand auf die Sprünge helfen:

Es gibt folgende Methode:

Code

Color Color::operator+
(
        const Color& c
) const
{


    Color a( coeff, n );
    Color b( c.coeff, n );


    Color aa( coeff, n );
    Color bb( c.coeff, n );


    return Color( a.coeff, n, 0, b.coeff );
}

Alles anzeigen

Der 4x verwendete Konstruktor sieht folgendermaßen aus:

Code

Color::Color
(
        const float* coeff,
        dword n
)
{
    this->coeff = new float[n];
    this->n = n;
    for( dword i = 0; i < n; i++ )
        this->coeff[i] = coeff[i];
    colorOp = COLOROP;
    type = TYPE;
}

Alles anzeigen

weiter oben (aber hoffentlich irrelevant ...):

Code

#define COLOROP ColorOp::allPrecInstance
#define TYPE 'a'

So, nun zum Phänomen: Wenn ich die Zeilen mit aa und bb, die ja ohnehin nicht verwendet werden und keine Auswirkung haben sollen (die Variablen aa und bb werden nicht verwendet!!), weglasse, dann sieht das Ergebnis völlig anders aus.

Unterschiedliche Ergebnisse:

Der Inhalt kann nicht angezeigt werden, da er nicht mehr verfügbar ist.

Wäre voll super wenn jemand den Durchblick hätte und mir weiterhelfen könnte!

Danke!

LG
Christian

Christoph R.

Ich würde den Fehler nicht in obigem Code suchen, denn wie du selbst sagst: es macht keinen Unterschied.

Eher glaube ich dass du irgendwo auf nicht allokierten Speicher zugreifst. Schau mal durch ob du eh nichts freigibst das du noch brauchst, und ob alle Pointer auf gültige Daten verweisen. Ich könnte mir vorstellen, dass das eben irgendwo nicht der Fall ist. Das kann auch ein ganz anderer Teil vom Programm sein (--> suchen :shiner:). Vermutlich kommt es dann durch die 2 Zeilen mehr oder weniger im Code zu irgendwelchen Verschiebungen von Daten im Speicher, die genau diese Effekte erzeugen.

Solche Fehler hatte ich auch gelegentlich als ich mich noch mit Computergraphik beschäftigt habe.

Plantschkuh!

Zitat von chni

Code

Color Color::operator+
(
        const Color& c
) const
{
    /* ... */
    return Color( a.coeff, n, 0, b.coeff );
}

Ich glaub Christoph hat Recht mit seiner Vermutung, daß du einen Speicherfehler hast. Konkret würde ich mir genau anschauen, was hier passiert: Der operator+ gibt die Kopie eines temporären Objekts zurück. Je nachdem, wie der Kopierkonstruktur und der Destruktor von Color definiert sind, kann es da schnell mal sein, daß:

für das temporäre Objekt ein neues Array angelegt wird;
der Zeiger auf dieses Array in den Rückgabewert kopiert wird;
der Destruktor des temporären Objekts das Array wieder freigibt, aber
der Rückgabewert noch immer einen, jetzt ungültigen, Zeiger auf dieses Array enthält.

Mit ein paar print-Statements (oder auch sophisticatederen Debuggingmethoden) sollte sich dieser Verdacht schnell überprüfen lassen.

Du brauchst übrigens keine Schleife zu schreiben, um die Elemente eines Arrays zu kopieren. Dafür gibts memcpy.

Edit: Aus obengenanntem neu-anlegen-und-kopieren-Grund ist es übrigens nicht nur fehleranfällig, sondern auch weniger effizient, den operator+ und Konsorten zu überladen; wenn es sich für dich algorithmisch irgendwie anbietet, definier lieber den destruktiven operator+= . Damit fallen viele solcher Probleme weg, und schneller ist es eben auch. (Ich bin zwar keiner, der meint, man müsse immer auf Teufel komm raus händisch optimieren, ohne auch nur festgestellt zu haben, daß das Programm "zu langsam" ist. Aber ich schätze Computergraphik kann nie schnell genug sein :))

Wenn man nur operator+= hat und doch auch mal operator+ bräuchte, kann man den Effekt von

Code

C = A + B

noch immer mit

Code

C = A;
C += B;

simulieren.

chni

Vielen Dank für eure Hinweise.

Ich habe das Programm nun nochmals auf solche Kleinigkeiten überprüft, obwohl ich von Anfang an sehr sorgsam und defensiv vorgegangen bin (daher auch Schleifen statt memcpy! Geschwindigkeit ist erstmal nebensächlich wenn die Mühle nicht einwandfrei läuft). Die Klasse Color wird nie mit new angelegt und alle Color-Konstruktoren enthalten new für die einzige Zeigervariable coeff. Irgendwie drehe ich mich im Kreis bei der Fehlersuche ...

Ich suche weiter ...

Plantschkuh!

Zitat von chni

und alle Color-Konstruktoren enthalten new für die einzige Zeigervariable coeff.

Auch der Kopierkonstruktor, der implizit definiert wird, wenn du dich nicht darum kümmerst? Auch der operator=, der implizit definiert wird, wenn du dich nicht darum kümmerst?

chni

Die beiden sehen wie folgt aus:

Code

Color::Color(const Color& color)
{
    coeff = new float[color.n];
    for( dword i = 0; i < color.n; i++ ) // TODO replace this by std::copy
        coeff[i] = color.coeff[i];
    n = color.n;
    colorOp = color.colorOp;
    type = color.type;
    skip = color.skip;
}

Code

Color& Color::operator=( const Color& color )
{
    if( this != &color ) // protect against self-assignment
    {
        // 1  allocate and copy
        float* coeff = new float[color.n];
        for( dword i = 0; i < color.n; i++ ) // TODO replace this by std::copy
            coeff[i] = color.coeff[i];


        // 2  delete old contents of this
        delete[] this->coeff;
        this->coeff = 0;


        // 3  point to new contents
        this->coeff = coeff;
        n = color.n;


        colorOp = color.colorOp;
        type = color.type;
        skip = color.skip;


    }
    return *this;
}

Alles anzeigen

LG
Christian

Plantschkuh!

Hmm, ja, die schauen gut aus. Aber im Kopierkonstruktur solltest du dich auch darum kümmern, coeff freizugeben. Das hat aber keinen Einfluss auf die Korrektheit.

Wenn das unter Linux wäre, würde ich ja empfehlen, die Sache mal mit valgrind laufen zu lassen. Aber ich vermute fast, daß du unter Windows unterwegs bist...

chni

Ich arbeite mit Ubuntu, Valgrind wäre also möglich. Allerdings fehlt mir da die Erfahrung mit der Bedienung. Gprof habe ich für einfache Geschwindigkeitsvergleiche verwendet.

Aber ich wusste nicht, dass ich im Kopierkonstruktor etwas freigeben muss. Welches coeff wäre das? this->coeff oder color.coeff?

Christoph R.

Freigeben musst du this->coeff, weil danach ja ein neues Array allokiert wird, so wie du das auch im Zuweisungsoperator machst.

Macht dein Programm sonst noch irgendwas? Versuche mal _nur_ Color zu instanzieren. Wie schon gesagt, kann auch ein ganz anderer Programmteil das Problem verursachen. Zumindest ist mir das schon passiert.

chni

Ist das wirklich wahr? Soweit ich weiß existiert kein Array wenn der Konstruktor aufgerufen wird, denn der ist ja erst dafür zuständig eines anzulegen! Beim Zuweisungsoperator ist das etwas anderes, der ist ja kein Konstruktor. Auch verschiedenste Quellen im Web inklusive Wikipedia schreiben nichts von einem Freigeben: http://de.wikipedia.org/wiki/Kopierkonstruktor

Sonst macht das Programm einiges, es ist ein einfacher Raytracer. Allerdings habe ich im Wesentlichen nur die Klasse Vector3f durch die Klasse Color ersetzt, der Raytracer ist von jemand anderem ( http://www.hxa.name/minilight/ ).

LG
Christian

Christoph R.

Zitat von chni

Ist das wirklich wahr? Soweit ich weiß existiert kein Array wenn der Konstruktor aufgerufen wird, denn der ist ja erst dafür zuständig eines anzulegen! Beim Zuweisungsoperator ist das etwas anderes, der ist ja kein Konstruktor.

Stimmt, da hast du recht.

Plantschkuh!

Zitat von chni

Ich arbeite mit Ubuntu, Valgrind wäre also möglich. Allerdings fehlt mir da die Erfahrung mit der Bedienung.

Ich verwend Valgrind immer so einfach wie möglich: Statt

Code

programm arg1 ... argn

ruf ich auf der Kommandozeile

Code

valgrind programm arg1 ... argn

auf. Fertig Es zahlt sich aus, das Programm mit Debuginformation zu kompilieren (beim gcc -g oder -ggdb). Dann kriegt man halt von Valgrind, während es dein Programm ausführt, immer wieder Meldungen darüber, daß an Stelle X irgendwas uninitialisiert verwendet wurde, oder an Stelle Y freigegebener Speicher verwendet wurde.

Es braucht ein bissi Übung, den Output zu lesen, aber es ist gar nicht so schlimm. Und scheinbar gibts auch ein Flag, der einen bei so einem Fehler direkt in den Debugger schmeißt. Hab ich aber noch nie verwendet.

Zitat von chni

Ist das wirklich wahr? Soweit ich weiß existiert kein Array wenn der Konstruktor aufgerufen wird, denn der ist ja erst dafür zuständig eines anzulegen!

Ja, ich glaub das war ein brain fart meinerseits. Kannst du wohl ignorieren.

Jetzt mitmachen!