Vereinigung von Listen
-
hab mir den Code nochmal angeguckt. Was du da in der Methode verein() machst ist eine lokale(!) Instanz und von der gibst du nur eine flache Kopie zurück, da du keinen Zuweisungsoperator überladen hast [Kset &operator=(const Kset &)].
Der Rest deiner Instanz wird durch deinen Destructor beim Verlassen der Methode zerblasen ...
-
Sepp Brannigan schrieb:
hab mir den Code nochmal angeguckt. Was du da in der Methode verein() machst ist eine lokale(!) Instanz und von der gibst du nur eine flache Kopie zurück, da du keinen Zuweisungsoperator überladen hast [Kset &operator=(const Kset &)].
Der Rest deiner Instanz wird durch deinen Destructor beim Verlassen der Methode zerblasen ...an dem code ist eine menge falsch - diese begründung hier allerdings ebenfalls zu 100%.
- zunächsteinmal ist diese liste ein speicherfass ohne boden - denn knoten werden nie freigegeben (folglich besteht obiges problem bzgl. des destruktors nicht wirklich).
- der copy-konstruktor sollte eine referenz auf const anstatt einer referenz auf non-const nehmen.
- im copy konstruktor wird das feld anzahl nicht kopiert (das verursacht vermutlich den absturz), nebenbei bemerkt ist ein solches feld für listen optional, es sollte eigentlich für keine der grundlegenden operationen benötigt werden. der test this!=&s ist im copy-construktor sinnlos.
- ein copy-assignment operator fehlt (wie erwähnt) und MUSS existieren wenn das destruktor problem gelöst wurde
- der operator + (implementierung hier nicht gezeigt) hat den falschen return-typ (wenn wir mal davon ausgehen, dass es auf verein() basiert), und sollte ebenfalls referenzen auf const nehmen
- view_all sollte const member seines sind sicher noch mehr probleme drin...
-
camper schrieb:
an dem code ist eine menge falsch - diese begründung hier allerdings ebenfalls zu 100%.
stimm ich dir doch glatt zu: Hier ist ne Menge faul. Allerdings würd mich interessieren, was an meiner Begründung so 100%ig falsch sein soll?
Nur mal angenommen, es gibt sonst keine Fehler allerdings auch keinen operator=() - was passiert deiner Meinung nach beim Verlassen der Methode?
-
Sepp Brannigan schrieb:
camper schrieb:
an dem code ist eine menge falsch - diese begründung hier allerdings ebenfalls zu 100%.
stimm ich dir doch glatt zu: Hier ist ne Menge faul. Allerdings würd mich interessieren, was an meiner Begründung so 100%ig falsch sein soll?
Nur mal angenommen, es gibt sonst keine Fehler allerdings auch keinen operator=() - was passiert deiner Meinung nach beim Verlassen der Methode?op= wird in verein() nie aufgerufen (der copy-ctor dagegen wenigstens einmal). abgesehen davon, solange der destruktor eben nicht richtig funktioniert, spielt es schlicht keine rolle, ob ein selbstdefinierter operator vorhanden ist - der implizit benutzte op= könnte durchaus 'korrekt' funktionieren. insofern ist die begründung mittels op= falsch.
-
Hallo, Ich hab das ganze jetzt nochmal umgeschrieben:
er läuft jetzt durch, zeigt aber die neue Liste, die die beiden anderen vereinigt falsch an:dekl:
#include <iostream> #include <conio> class Kset { private: struct Knoten { int Inhalt; Knoten *pNext; }; int Anzahl; Knoten *pFirst; Knoten *pLast; public: Kset(); Kset(const Kset & obj); Kset verein(const Kset & s); ~Kset(); void insert_front(int obj); void view_all(); };impl:
#include "dekl11.h" Kset::Kset() { Anzahl = 0; pFirst = pLast = NULL; } Kset::~Kset() { Knoten *ptemp = pFirst; for(; Anzahl>0; Anzahl--) { pFirst = pFirst->pNext; delete ptemp; ptemp = pFirst; Anzahl--; } } Kset::Kset(const Kset & obj) { this->Anzahl = 0; pFirst = pLast = NULL; Knoten *ptemp = obj.pFirst; for(int anz = obj.Anzahl; anz != 0; anz--) { this->insert_front(ptemp->Inhalt); ptemp = ptemp->pNext; } } Kset Kset::verein(const Kset & s) { Kset tmp(*this); for(Knoten *ptemp=s.pFirst; ptemp !=NULL; ptemp = ptemp->pNext) tmp.insert_front(ptemp->Inhalt); return tmp; } void Kset::insert_front(int obj) { if(Anzahl == 0) { pFirst = new Knoten; pFirst->Inhalt = obj; pFirst->pNext = NULL; pLast = pFirst; } else { Knoten *pNew = new Knoten; pNew->Inhalt = obj; pNew->pNext = pFirst; pFirst = pNew; } Anzahl++; } void Kset::view_all() { Knoten *pTemp = pFirst; for(int anz=Anzahl; anz>0;anz--) { cout<<pTemp->Inhalt<<" "; pTemp = pTemp->pNext; } }main:
#include "dekl11.h" #pragma hdrstop //--------------------------------------------------------------------------- #pragma argsused int main(int argc, char* argv[]) { Kset k; k.insert_front(1); k.insert_front(2); k.insert_front(3); k.insert_front(4); k.view_all(); cout<<endl<<endl<<endl; Kset k2(k); k2.insert_front(9); k2.insert_front(8); k2.view_all(); cout<<endl<<endl; Kset k3; k3=k.verein(k2); k3.view_all(); getch(); return 0; } //---------------------------------------------------------------------------
-
Mr. Blonde schrieb:
Hallo, Ich hab das ganze jetzt nochmal umgeschrieben:
er läuft jetzt durch, zeigt aber die neue Liste, die die beiden anderen vereinigt falsch an:Was genau meinst du mit "zeigt falsch an"? verein() nimmt die elemente von s und hängt sie jeweils VORNE an this an
k: 4 3 2 1 k2: 8 9 4 3 2 1 -> k3: 1 2 3 4 9 8 4 3 2 1(das sollte nach meinem Verständnis rauskommen)
-
Hi!
Anzahl--;Mach das raus aus der Schleife im Destruktor.
Und gibt da:
Kset Kset::verein(const Kset & s)eine Referenz auf *this zurück, wobei du *this änderst und keine lokale Kopie erstellst.
grüße
-
camper schrieb:
Sepp Brannigan schrieb:
camper schrieb:
an dem code ist eine menge falsch - diese begründung hier allerdings ebenfalls zu 100%.
stimm ich dir doch glatt zu: Hier ist ne Menge faul. Allerdings würd mich interessieren, was an meiner Begründung so 100%ig falsch sein soll?
Nur mal angenommen, es gibt sonst keine Fehler allerdings auch keinen operator=() - was passiert deiner Meinung nach beim Verlassen der Methode?op= wird in verein() nie aufgerufen (der copy-ctor dagegen wenigstens einmal). abgesehen davon, solange der destruktor eben nicht richtig funktioniert, spielt es schlicht keine rolle, ob ein selbstdefinierter operator vorhanden ist - der implizit benutzte op= könnte durchaus 'korrekt' funktionieren. insofern ist die begründung mittels op= falsch.
Ich schrieb: beim Verlassen der Methode - also beim return (noch genauer: vor dem Lauf des Destructors) und da wird der operator=() wohl aufgerufen [M3 = M1.verein(M2);]. Daher spielt dieser durchaus eine Rolle - die, wenn nicht beachtet, _definitiv_ zu Problemen führt und zwar mit oder ohne funktionierendem Destructor. Ausserdem wage ich zu behaupten: "... könnte der implizit benutzte op= durchaus 'korrekt' funktionieren" ist hier wohl das, was sicherlich _nicht_ richtig ist - der implizite op=() erstellt dir eine flache kopie - was nicht ausreicht - soviel dürfte klar sein.
Darüberhinaus hab ich nie behauptet, alle Fehler zu listen. Die Frage war, "woran kann das liegen?" - und das ist IMHO eine der Möglichkeiten.
Also nochmal: Wenn ich wirklich falsch liege, dann bitte gib mir ne plausible Erklärung (das eben war keine) - immerhin behauptest du hier, dass ich bullshit erzähle ...grüße
-
CStoll schrieb:
Mr. Blonde schrieb:
Hallo, Ich hab das ganze jetzt nochmal umgeschrieben:
er läuft jetzt durch, zeigt aber die neue Liste, die die beiden anderen vereinigt falsch an:Was genau meinst du mit "zeigt falsch an"? verein() nimmt die elemente von s und hängt sie jeweils VORNE an this an
k: 4 3 2 1 k2: 8 9 4 3 2 1 -> k3: 1 2 3 4 9 8 4 3 2 1(das sollte nach meinem Verständnis rauskommen)
jup - sollte es ... aber es kommt eine komische zahlenfolge raus:
845641244 1 42257 1 42257 1 42257 1 42257 1
-
Dann dürfte wohl das Problem mit dem falschen Zuweisungsoperator dich gerade erschlagen haben
- tmp ist eine lokale Variable in verein() und wird am Ende gelöscht - k3 bekommt eine flache Kopie davon (nur Anzahl und pFirst/pLast-Zeiger). Da tmp's Destruktor die Daten hinter pFirst/pLast vernichtet hat, rennst du im Endeffekt durch Speicherbereiche, in denen irgendwas stehen könnte (mit sehr viel Glück wurden die Datenbereiche deiner Liste "nur" als verwendbar markiert, aber wahrscheinlicher ist, daß schon jemand anderes sie in Beschlag genommen hat).
-
Hi!
Ich sag ja, machs so:
List &List::merge( const List &other ) { Node *n = other.head; for ( ; n; n = n->next ) push_back( n->value ); //*this wird geändert return *this; // Referenz auf this zurückgeben }Dann hast du das Problem nicht.
grüße
-
ahhh ... jetzt verstehe ich - thx!!!
-
Sepp Brannigan schrieb:
camper schrieb:
Sepp Brannigan schrieb:
camper schrieb:
an dem code ist eine menge falsch - diese begründung hier allerdings ebenfalls zu 100%.
stimm ich dir doch glatt zu: Hier ist ne Menge faul. Allerdings würd mich interessieren, was an meiner Begründung so 100%ig falsch sein soll?
Nur mal angenommen, es gibt sonst keine Fehler allerdings auch keinen operator=() - was passiert deiner Meinung nach beim Verlassen der Methode?op= wird in verein() nie aufgerufen (der copy-ctor dagegen wenigstens einmal). abgesehen davon, solange der destruktor eben nicht richtig funktioniert, spielt es schlicht keine rolle, ob ein selbstdefinierter operator vorhanden ist - der implizit benutzte op= könnte durchaus 'korrekt' funktionieren. insofern ist die begründung mittels op= falsch.
Ich schrieb: beim Verlassen der Methode - also beim return (noch genauer: vor dem Lauf des Destructors) und da wird der operator=() wohl aufgerufen [M3 = M1.verein(M2);].
oh ja, stimmt - da hab ich gar nicht hingeschaut. macht aber nichts - eine flache kopie reicht völlig aus, da die knoten liste selbst ja intakt bleibt (solange der destruktor nicht funktinierte - klar).
Daher spielt dieser durchaus eine Rolle - die, wenn nicht beachtet, _definitiv_ zu Problemen führt und zwar mit oder ohne funktionierendem Destructor. Ausserdem wage ich zu behaupten: "... könnte der implizit benutzte op= durchaus 'korrekt' funktionieren" ist hier wohl das, was sicherlich _nicht_ richtig ist - der implizite op=() erstellt dir eine flache kopie - was nicht ausreicht - soviel dürfte klar sein.
Darüberhinaus hab ich nie behauptet, alle Fehler zu listen. Die Frage war, "woran kann das liegen?" - und das ist IMHO eine der Möglichkeiten.
Also nochmal: Wenn ich wirklich falsch liege, dann bitte gib mir ne plausible Erklärung (das eben war keine) - immerhin behauptest du hier, dass ich bullshit erzähle ...grüße
wie gesagt, der implizite op= reichte durchaus aus (er führt nur dazu, dass die kopie sich die knotenliste mit dem original teilt - und das führt möglicherweise zu problemen, sobald eine der listen verändert wird. dass der destruktor die anker pFirst,pLast löschte, spielt keine rolle. der wird schliesslich erst nach erstellen der kopie per copy-ctor bzw. copy-op ausgeführt (wir machen ja keine kopien von toten objekten). nat. brauchen wir einen eigenen op= wenn die liste jemals richtig funktionieren soll.
abgesehen davon, spät in der nacht lässt die hirnaktivität nach - es war keineswegs ein angriff auf dich.