Destruktor wird scheinbar grundlos aufgerufen

Jeremin

Da hab ich vor lauter Bäumen den Wald wohl nicht mehr gesehen, danke euch

Probier nicht rum, lern einfach C++.

Was denkst du, wozu mach das Ganze?

Gruss
jeremin

Darf man fragen mit welchem Buch du lernst?

Mechanics

Jeremin schrieb:

Probier nicht rum, lern einfach C++.

Was denkst du, wozu mach das Ganze?

Learning by doing hilft in der Phase nicht so viel, zumindest bei C++. Das muss man einfach wissen. Und bevor du nicht ein Grundlagenbuch (oder Tutorial oder was auch immer) durchgearbeitet hast, wirst du die ganze Zeit Probleme haben, die du nicht verstehst.

Jeremin

Ich versteh das Problem ja durchaus, ich hab nur übersehen, dass ich eine lokale Instanz von CFicture erzeugt habe. Es ist mir durchaus bewusst, dass der Destruktor nach dem Verlassen der Funktion aufgerufen wird, jetzt da ich die Ursache kenne.

Wobei das Problem evt. an meinem Programmierstil liegen könnte (obwohl ich versuche den zu bessern). Falls du den kritisieren möchtest, ich bitte darum

Aber ich bin ziemlich sicher, dass auch Leuten, die Tag täglich mit C++ zu tun haben noch Flüchtigkeitsfehler unterlaufen, wenn auch deutlich weniger häufig als mir.

Gruss
Jeremin

PS: Buch ist C++ Primer Plus, wobei dieses Projekt eher der Vertiefung des Gelernten dient.

Mechanics

Jeremin schrieb:

Wobei das Problem evt. an meinem Programmierstil liegen könnte (obwohl ich versuche den zu bessern). Falls du den kritisieren möchtest, ich bitte darum

Nee, da hab ich eigentlich nichts besonderes zu kritisieren. Außer vielleicht dass man Parameter möglichst als konstante Referenzen übergeben sollte. Das Programm ist viel zu klein, um irgendwelche Aussageb über den Stil machen zu können.
Du brauchst dir meine Kritik nicht so zu Herzen nehmen. Der Fehler war nur offensichtlich und deswegen hab ich gedacht, dir ist nicht klar, wie Stack Objekte funktionieren. Wenn das nicht der Fall ist, ist ja alles in Ordnung

Jeremin schrieb:

Wobei das Problem evt. an meinem Programmierstil liegen könnte (obwohl ich versuche den zu bessern). Falls du den kritisieren möchtest, ich bitte darum

Warum kopierst du die Pfade mit strcpy? Du hast doch schon patchPath.c_str() entdeckt, das kannst du auch direkt benutzen.

Hm, ein paar Gedanken zu dem code:
Warum verwendest du char*?

ifstream fin(patchPath.c_str(), ios::in | ios::binary);

geht doch genauso. Spart dir das new/delete.

fin.read((char*) &myPatchSaveFile, sizeof(patchSave));

(char*) als C-style cast bin ich nicht unbedingt ein fan von. Besser reinterpret_cast<char*>()

Als letztes kannst du dir noch überlegen (auch wenn man da geteilter Meinung sein kann) ob du deine Funktion nicht in 2 Teile teilen willst. Prinzipiell lädst du ja einmal das patch file, und einmal fictures, auch wenn das etwas zusammenhängt.

Ich meine damit in etwa sowas:

patchSave LoadPatchFile(string path)
{
path+="\\Patch.ptch";
patchSave myPatchSaveFile;

    //Patchinfos laden
    ifstream fin(path.c_str(), ios::in | ios::binary);
    if (fin.is_open()){
        fin.read((char*) &myPatchSaveFile, sizeof(patchSave));

        //Dimmer-Infos kopieren
        for (int i=0; i<NDIMCHANNELS; i++){
            this->dimCh[i] = myPatchSaveFile.dimCh[i];
            this->dimChAlias[i] = myPatchSaveFile.dimChAlias[i];
        }
        //Ficture-Infos kopieren
        for (int i=0; i<NFICTURES; i++){
            this->fictureAlias[i] = myPatchSaveFile.fictureAlias[i];
        }

        for (int i=0; i<512; i++)
            this->available[i] = myPatchSaveFile.available[i];
    }
    else
    {

       // returnValue -= 1;
       //Das geht so natürlich nicht mehr, ich würde hier eine exception werfen
    }
return myPatchSaveFile;

}

void LoadFictures(string path, const patchSave& myPatchSaveFile)
{
    path += "\\Fictures.fict";
    CFicture aFicture;

    //alle gespeicherten Fictures laden
    ifstream fin(path.c_str(), ios::in | ios::binary);
    if (fin.is_open()){
        for (int i=0; i<NFICTURES; i++){
            if (myPatchSaveFile.ficturesUsed[i]){
                fin.read((char*) &aFicture, sizeof(CFicture));
                if (fictures[i] != NULL)
                    delete fictures[i];
                fictures[i] = new CFicture(aFicture);
            }
            else{
                if (fictures[i] != NULL)
                    delete fictures[i];
                fictures[i] = NULL;
            }
        }
    }
    else
            //returnValue -= 2;
            //das würde hier zwar gehen, aber ich würde trotzdem eine exception bevorzugen
}

void CPatching::loadPatchingFile(string path){
   //falls du excepctions wirfst kannst du sie entweder hier fangen und als errorcode zurückgeben, oder gleich weiterfliegen lassen - soll sich doch der aufrufer drum kümmern. Wenn LoadPatchFile wirft, wird LoadFictures auch nicht ausgeführt, darauf muss man also nicht prüfen
   patchSave myPatchFile = LoadPatchFile(path);
   LoadFictures(path, myPatchFile);
}

Weiter würd ich den returnValue nicht mit -= zuweisen, da muss man sich ja immer den ganzen code anschaun, nur um zu wissen, was tatsächlich zurückgegeben wird. Besser direkt zuweisen - wenns geht.

Soll nur ein Anreiz zum nachdenken sein, gibt hier sicher 50 Leute die vorallem so ne Aufteilung für total dumm halten. :> Ich mags, weil es die einzelnen Aufgaben im code klarer darstellt. Andere sehen das sicher anders.

Michael E.

Sehe ich auch so mit der Aufteilung. Der nächste wichtige Schritt ist dann, aus CFicture **fictures ein vector<CFicture> fictures zu machen, sodass man die ganzen news und deletes los wird.

Jeremin

Gute Frage wieso ich char* verwende, ich glaube, ursprünglich habe ich es mit .c_str() gemacht, doch dann hab ich das aus irgendeinem Grund geändert (mal schauen, vielleicht finde ich raus, wieso).

fin.read((char*) &myPatchSaveFile, sizeof(patchSave));

Wir hatten das in der C++ Übungen so gelöst, daher ging ich davon aus, dass das ein vernünftiger Weg sei. Um reinterpret_cast habe ich bis jetzt einen Bogen gemacht, da dieser ja nicht ganz ungefährlich sei.

Die Aufteilung würde Sinn machen, dass baue ich in nächster Zeit um. Genauso mit exeptions, die sind eigentlich schon länger geplant, bin aber noch nicht dazu gekommen.

Der nächste wichtige Schritt ist dann, aus CFicture **fictures ein vector<CFicture> fictures zu machen, sodass man die ganzen news und deletes los wird.

Das Problem hierbei ist, dass ich explizit auf Fictures zugreifen möchte. Zum Beispiel möchte ich die Möglichkeit haben, auf Ficture4 zugreifen zu können, auch wenn vorher nach gar nichts gepatch ist.
Wenn ich das einem Vector lösen würde, müsste ich quasi die Versetzung von 4 auf 0 irgendwo mitführen, was mir recht umständlich vorkam.
Mir ist klar, dass CFicture **fictures; ein eher unsauberes Konstrukt darstellt, aber mir ist keine vernünftige Alternative dazu eingefallen.
Würdet ihr sagen, dass es sich lohnt, da noch etwas Hirnschmalz zu investieren und eine Lösung mit std::vector zu finden?

Danke für die Tipps
Jeremin

314159265358979

Jeremin schrieb:

Wir hatten das in der C++ Übungen so gelöst, daher ging ich davon aus, dass das ein vernünftiger Weg sei. Um reinterpret_cast habe ich bis jetzt einen Bogen gemacht, da dieser ja nicht ganz ungefährlich sei.

C-Casts sind noch viel gefährlicher als reinterpret_cast, da du damit auch CV-Qualifier wegcasten kannst.

Oder ums anders zu sagen - C-Style casts statt reinterpret_cast zu verwenden, weil dieser gefährlich ist, ist so ähnlich wie eine Atom-Bombe statt einer Splittergranate zu verwenden, weil die Splittergranate so stark explodiert.