Implementierung von operator=() mit Deep Copy einer dynamischen Liste

heimchen

Hi,

ich hab (unter Qt) eine Kalsse definiert, die unter anderen eine dynamische Liste enthält. Das ganze sieht so aus:

typedef struct myData {
  int value;
  struct myData* pNext;
} tMyData;

class cMyClass
{
public:
  int a;
  int b;
  int c;
  tMyData* myData;

    cMyClass();
    ~cMyClass();
    cMyClass & operator =(cMyClass value);
};

Die Liste wird über den Konstruktor und Destruktor natürlich entsprechend verwaltet.

Über den operator= möchte ich nun eine Kopie meiner Klasse erstellen:

cMyClass & cMyClass::operator =(cMyClass value)
{
  this->a = value.a;
  this->b = value.b;
  this->c = value.c;

  << Lösche this->pMyData >>

  for (tMyData* p=value.pItem; p; p=p->pNext) {
        tMyData* pNew = new tMyData;

        memcpy(pNew, p, sizeof(tMyData));

        pNew->pNext = 0;

        << Hänge pNew an this->pMyData an >>
  }

  return *this;
}

Wenn ich diesen Operator verwende, klappt aber das kopieren nicht. Im Debugger werden mir die Inhalte von pMyData der Quell- und Zielklasse als "<not accessible>" angezeigt. Mein Programm stürzt beim Zugriff auf den Inhalt hab.

Was mach ich falsch?

SeppJ

Ist das für dich selbst als Übungsaufgabe gedacht oder programmierst du hier unwissentlich std::list nach?

Da bei deinem bisherigen Ansatz mindestens noch ein Kopierkonstruktor fehlt würde ich erst einmal dort einen Fehler vermuten.

DocShoe

Du übergibst das zu kopierende Objekt per value. Ohne den Kopierkonstruktor zerschießt du dir da schon deine Objekte. Du verletzt hier die Regel der Großen Drei.
Muss es unbedingt eine selbstgestrickte Liste sein? Die STL bietet mit Sicherheit einen passenden Container, der deinen Anforderungen genügt.

heimchen

Bin eigentlich C-Programmierer. Schreib mir nur grad ein kleines Tool, weil mich das permanente von-Hand machen imeer der gleichen Sachen aufgeregt hat

Mal so eine dumme Frage: was passiert denn durch den Aufruf per value? Im Prinzip reicht mir doch eine Kopie aller privaten Eigenschaften...

Wenn ich statt per value nun per reference verwende, müsste sich das Problkem doch eigentlich erledigt haben, oder?

Undabhängig davon, ob es schon eine fretige Lösung gibt

DocShoe

Bei call-by-value wird eine Kopie des Objektes erstellt und diese Kopie als Funktionsparameter verwendet. Da du keinen Kopierkonstruktor für deine Klasse hast werden die Elemente bitweise kopiert. Nach dem Verlassen der aufgerufenen Funktion wird die Kopie wieder zerstört (und damit der Destruktor aufgerufen). Jetzt kannst du ja selber überlegen, was dann mit deinem Originalobjekt passiert ist.

bmario

heimchen schrieb:

Bin eigentlich C-Programmierer. Schreib mir nur grad ein kleines Tool, weil mich das permanente von-Hand machen imeer der gleichen Sachen aufgeregt hat

Du regst dich auf, dass du immer die gleichen Sachen per Hand machen musst, aber willst selber eine Liste programmieren?

Nimm std::list oder QList, da du ohnehin schon Qt benutzt.

SeppJ

Oder nimm keine Liste, sondern was flottes.

krümelkacker

heimchen schrieb:

Über den operator= möchte ich nun eine Kopie meiner Klasse:

Bei einer Klasse wie dieser, ist es angebracht, den Zuweisungsoperator so zu definieren:

DeineKlasse& operator=(DeineKlasse temp)
      {
        this->swap(temp); // Dieser Trick nennt sich
        return *this;     // "Copy-and-Swap", temp ist die Kopie
      }

Dazu benötigst Du natürlich einen funktionierenden Kopierkonstruktor (für temp) und eine entsprechende swap-Funktion (da kannst Du dann einfach intern zwei Zeiger vertauschen oder sowas). Dazu solltest Du dann noch

inline void swap(DeineKlasse& a, DeineKlasse& b)
{ a.swap(b); }

als freie Funktion in demselben Namensraum anbieten, damit sie per ADL aka Koenig Lookup erreichbar ist.

Du kannst es Dir natürlich wesentlich einfacher machen, indem Du "richtig" C++ programmierst (im Sinne von effektiver Nutzung der Dir zur Verfügung stehenden Mittel):

class NochNeKlasse
{
  std::list<int> intliste_;  // oder doch besser std::vector<int> ?
  int a,b,c;
public:
  …
};

Hier ist kein von Hand geschriebener Kopierkonstruktor, Destruktor oder Zuweisungsoperator nötig. Das kommt daher, da std::list<> schon Deine gewünschte Kopiersemantik besitzt und nicht nur ein dämlicher Zeiger ist. Die für NochNeKlasse generierten Kopieroperationen des Compilers "kopieren" auch nur die Datenelemente. Und was "Kopieren" bei std::list heißt, wird ja in dem Kopierkonstruktor der std::list definiert.

heimchen schrieb:

Was mach ich falsch?

Du programmierst drauf los. Kapselung geht in C++ viel weiter als in C. C++ erlaubt Dir einfach zu benutzende Typen und Typfamilien selbst zu definieren (inklusive eigener Erzeuge-, Zerstör- und Kopier-Semantik). Einiges von solchen Typen hält die Standardbibliothek bereit. Nutze sie. Das heißt aber nicht, dass Du bei allen Deinen Klassen, Kopierkonstruktor, Destruktor etc selbst definieren musst. Versuche, sie korrekt vom Compiler generieren zu lassen und nur für die kleinsten Bausteine selbst zu definieren, falls nötig. Ein wichtiges Designprinzip ist, dass sich eine Klasse (in Form von selbst geschriebenen Verwaltungsoperationen a la Kopierkonstruktor etc) nur um höchstens eine Ressource kümmern sollte. Wenn Du also irgendwann zwei Listen brauchen solltest, dann wär es extrem praktisch, das Konzept "Liste" als eigenen Typ vorrätig zu haben, statt dass die Klasse, die 2 Listen benötigt, alles selbst machen muss…

krümelkacker

DocShoe schrieb:

Da du keinen Kopierkonstruktor für deine Klasse hast werden die Elemente bitweise kopiert.

Bei dieser Klasse, ja. Im Allgemeinen werden die Datenelemente "kopiert", wobei "kopieren" das ist, was der entsprechende Typ des Datenelements definiert — also nicht unbedingt bitweise…

heimchen

Ok, ich hab jetzt QList genommen, war schon recht bequem
Jetzt stürzt auch mein Programm nicht mehr ständig ab. Hatte den Destruktor nicht im Blick, der mir die Listenelemente gelöscht hat. Und wenn das schon bei der Kopie passiert ist, gibt das komische Effekte beim Original...

bmario schrieb:

Du regst dich auf, dass du immer die gleichen Sachen per Hand machen musst, aber willst selber eine Liste programmieren?

Mit "von Hand" meinte ich tatsächlich von Hand, also einmal Daten in ein Programm eingeben und ein zweites Mal die gleichen Daten in ein anderes Programm eingeben. Das Programmieren einer eigenen Liste ist dazu schon ein immenser Fortschritt. Abgesehen davon find ich das auch ganz interessant.

314159265358979

Wie man eine ordentliche einfach verkettete Liste implementiert, siehst du hier: http://www.c-plusplus.net/forum/p2156719#2156719

heimchen

Habs jetzt nur mal überflogen, aber dort ist auch kein Kopierkonstruktor drin. Da dürfte es ähnliche Probleme geben, wie bei mir.
Abgesehen davon gehören Listen ja auch zum Lehr-Standard in Schule und Uni, also im Prinzip weiß ich, wie's geht. Probleme machen ja erst die eigenen Spezialitäten.

314159265358979

Ist er wohl. Lern lesen.

krümelkacker

314159265358979 schrieb:

Wie man eine ordentliche einfach verkettete Liste implementiert, siehst du hier: http://www.c-plusplus.net/forum/p2156719#2156719

So toll finde ich die jetzt nicht. Mich stört, dass da immer -- auch bei einer leeren Liste -- zwei T-Objekte erzeugt werden und T dementsprechend default-konstruierbar sein muss.

Vielleicht könnte man das noch mit Vererbung so lösen

struct list_link
{
  list_link* next;

  explicit list_link(list_link* n = 0)
  : next(n)
  {}

  list_link(list_link const&) = delete;
  list_link& operator=(list_link const&) = delete;
};

template<class T>
struct list_node : list_link
{
    T value;

    list_node* next() const {return static_cast<list_node*>(list_link::next);}
    void next(list_node* n) {list_link::next = n;}

    template<class...Args>
    explicit list_node(list_node* n, Args&&...args)
    : list_link(n)
    , value(std::forward<Args>(args)...)
    {}
};

und dann liste so aufbauen:

template<class T>
class list
{
  list_link  before_begin_;
  list_link *before_end_ptr_; // fuer schnelles push_back
public:
  list()
  : before_begin_(/* mit next= */ 0)
  : before_end_ptr_(&before_begin_)
  {}
  ...
};

Die Verkettungsoperationen (Knoten löschen, einfügen, etc) könnte man auch auf Basis von list_link definieren, weil diese Funktionen ja auch nicht von T abhängen. Hier und da müsste man ggf noch static_cast<node<T>*> auf einen next-Zeiger anwenden. Das ist aber IMHO vertretbar; denn in diesem Design würde jeder next-Zeiger garantiert auf ein node<T>-Objekt zeigen (oder 0 sein).

Ach, ist "last" wirklich nötig? Das Ding scheint mir im Moment sehr nutzlos zu sein, zumindest bei einer einfach verkettenen Liste.
Edit: Ok, das weißt Du schon, sehe ich gerade.

list_link* remove_link(list_link* predecessor) {
  list_link* removed = predecessor->next;
  predecessor->next = removed->next;
  removed->next = 0;
  return removed;
}

oder so...

(alles ungetestet)

Cheers!
kk

P.S.: Sei mal nicht so patzig. Der Überlegenheitskomplex steht Dir nicht. :-p

314159265358979

Ist lesen denn so schwer? Meine Fresse.

krümelkacker

314159265358979 schrieb:

Ist lesen denn so schwer? Meine Fresse.

Nö. Habe keine Lust den kompletten Fred zu lesen. Du verlinkst auf Deine Liste. Die habe ich kritisiert. Nichts ist daran verwerflich. Sollte da am Ende des Freds noch eine andere bessere Implementierung stehen, hättest Du die ja verlinken können...

Falls ich mich jetzt zu unrecht angesprochen gefühlt habe: Ja, den Kopierkonstruktor kann man schonmal übersehen -- gerade dann, wenn er nicht auf dieser Seite sondern auf einer der folgenden kommt. Das ist kein Grund so pampig zu werden, meiner Meiung nach.

314159265358979

Wenn 2 Leute hintereinander ganz offensichtlich nicht lesen können...

Shade Of Mine

314159265358979 schrieb:

Wenn 2 Leute hintereinander ganz offensichtlich nicht lesen können...

Und wenn du nur einen funken intelligenz hättest...

krümelkacker

314159265358979 schrieb:

Wenn 2 Leute hintereinander ganz offensichtlich nicht lesen können...

Du trollst doch nur rum, gerade. Das ist mir jetzt nicht mehr als 2 Sätze wert.

DocShoe

Bin ich der einzige, den ein (temporärer) pi-Bann nicht stören würde?