eigene Methode zur Objektkopie?

SBond

Hallo Leute,

Folgende Situation: Ich habe ein Objekt, welches ich gerne kopieren möchte. Ich verwende C++ 11 und das Objekt beinhaltet verschiedene Strukturen und nutzt dynamischen Speicher. Zum Kopieren des Objekts verwende ich copy constructor und copy assignment. Soweit so normal. Nun ist mein Kopierkonstruktor recht umfangreich und ich könnte natürlich den Code fast 1:1 im Zuweisungsoperator kopieren, aber ich denke da gibt es eine bessere Lösung, bei der man nicht den Code duplizieren muss. Nach lagem Suchen stieß ich immer wieder auf ein 'copy&swap'-Idiom. Habe mir ein paar Beispiele angeschaut und in meiner Klasse angewendet. Das Ergebnis sah dann so aus:

MyClass &MyClass::operator=(const MyClass &other)
{
    if (this != &other)           // protect against invalid self-assignment
    {
    	MyClass temp(other);      // call the copy constructor
        std::swap(*this, temp);   // swap the values
    }
    return *this;
}

Das Objekt 'temp' ist eine erfolgreiche Kopie, die über den Kopierkonstruktor erzeugt wird. Allerdings stürzt mein Programm bei der Zeile 'std::swap(...)' sofort ab. Ich dachte dass der Zeiger von 'temp' und '*this' irgendwie getauscht werden, so dass 'this' fortan auf das Objekt 'temp' referenziert.

Ich habe auch Beispiele gesehen, in denen 'swap' einen Parameter hatte und manuell implementiert wurde. Dabei wurden alle Elemente einzeln mit 'swap' getauscht. Da frage ich mich eigendlich, wo man denn Programmieraufwand spart.

Ich habe es jetzt wie folgt gelöst: Ich habe mit eine Methode definiert:

void MyClass::copy(MyClass &dest, const MyClass &source)
{
  ...
}

In der befindet sich die Kopierroutine. Diese Methode rufe ich nun im Kopierkonstruktor und beim Zuweisungsoperator auf:

MyClass::MyClass(const MyClass &other)
: // ... dynamischen Speicher allozieren
{
	this->copy(*this, other);
}

MyClass &MyClass::operator=(const MyClass &other)
{
    if (this != &other)
    {
    	this->copy(*this, other);
    }
    return *this;
}

copy() ist die Kopierfunktion. Das ganze funktioniert prima, aber ist es auch gut so oder hätte ich es besser lösen können?
...und was mache ich am besten, wenn die Bedingung 'if (this != &other)' false ist? Einfach so lassen oder besser eine exception werfen?

viele Grüße,
SBond

dot

SBond schrieb:

Nun ist mein Kopierkonstruktor recht umfangreich und ich könnte natürlich den Code fast 1:1 im Zuweisungsoperator kopieren, aber ich denke da gibt es eine bessere Lösung, bei der man nicht den Code duplizieren muss.

Wie wär's mit copy-and-swap?

SBond schrieb:

...und was mache ich am besten, wenn die Bedingung 'if (this != &other)' false ist? Einfach so lassen oder besser eine exception werfen?

Die viel wichtigere Frage ist erstmal: Wieso brauchst du diese Abfrage überhaupt erst?

Torsten Robitzki

Hallo S.

SBond schrieb:

Nun ist mein Kopierkonstruktor recht umfangreich und ich könnte natürlich den Code fast 1:1 im Zuweisungsoperator kopieren, aber ich denke da gibt es eine bessere Lösung, bei der man nicht den Code duplizieren muss.

Beste Möglichkeit wäre wahrscheinlich mal zu gucken, warum der copy c'tor so umfangreich ist, bzw. warum ein einfaches Delegieren an die member nicht ausreichend ist.

SBond schrieb:

Nach lagem Suchen stieß ich immer wieder auf ein 'copy&swap'-Idiom. Habe mir ein paar Beispiele angeschaut und in meiner Klasse angewendet. Das Ergebnis sah dann so aus:

Der Test auf self-assigment ist überflüssig, da copy & swap wegen seiner Transaktions-Struktur auch mit self-assignment zurecht kommt (das Objekt wird in keine Zwischenzustände gebracht).

Wenn Du Deinen Abstürz genauer untersucht hättest, dann hättest Du wahrscheinlich festgestellt, dass es sich um einen Stackoverflow handelt, da Deine Implementierung von operator=() die standard-Implementierung von std::swap verwendet, welche wiederum die Zuweisung verwendet. Die Lösung ist:

SBond schrieb:

Ich habe auch Beispiele gesehen, in denen 'swap' einen Parameter hatte und manuell implementiert wurde. Dabei wurden alle Elemente einzeln mit 'swap' getauscht. Da frage ich mich eigendlich, wo man denn Programmieraufwand spart.

Den nötigen "Programmieraufwand" hast Du Dir durch Deinen komplexen Klassenaufbau "eingebrockt". Copy&Swap möchte nicht den "Programmieraufwand" minimieren, sondern die "strong exception guarantee" für die Implementierung des Zuweisungsoperators geben. Und dazu ist es nötig, dass swap() keine exception wirft und das erreicht man in der Regel durch das "Swappen" der Member ohne Funktionsaufrufe, die exceptions werfen können (keine Speicher-Alloziierung etc.).

Die "übliche" Form von copy & swap wäre also (einher gehend mit einer Überladung von std::swap() für MyClass):

MyClass &MyClass::operator=(MyClass other)
{
    swap( other );
    return *this;
}

SBond schrieb:

copy() ist die Kopierfunktion. Das ganze funktioniert prima, aber ist es auch gut so oder hätte ich es besser lösen können?

Du hast immer drei Instanzen von MyClass in Deiner copy-Funktion (this und die beiden Argumente). Eine davon ist überflüssig. Wenn Du die Funktion danach in operator= umbenennst, dann bist Du bei der gleichen Lösung und delegierst einen Teil der Implementierung von copy Konstruktur an den Zuweisungsoperator, in dem Du ein Objekt konstruierst und danach Zuweist.

SBond schrieb:

...und was mache ich am besten, wenn die Bedingung 'if (this != &other)' false ist? Einfach so lassen oder besser eine exception werfen?

int a = 5;
a = a; // würdest Du an dieser Stelle eine Exception erwarten?

mfg Torsten

Nathan

Bei deiner copy() Funktion: wer gibt den alten Speicher frei?

Zeig mal deine Member, wenn du Smartpointer nutzt, entfällt schon mal viel Arbeit.Ebenso durch das Single Responsibility Prinzip.

SBond

Erstmal vielen Dank für eure Unterstützung

Torsten Robitzki schrieb:

Du hast immer drei Instanzen von MyClass in Deiner copy-Funktion (this und die beiden Argumente).

Das war mir nicht bewusst. Da ich ja '*this' als Parameter übergeben habe, dachte ich, dass auch nur '*this' und 'other' instanziiert wurden. Mit copy-and-spap hatte ich bisher noch nicht gearbeitet, daher habe ich wohl noch leichte Verständnisprobleme zu diesem Thema. Werde mir demnächst vernünftige Bücher besorgen. Mein aktuelles Material ist ziemlich mager.

Nathan schrieb:

Bei deiner copy() Funktion: wer gibt den alten Speicher frei? Zeig mal deine Member, ....

Der Speicher wird vom Destruktor freigegeben. Speicherlecks habe ich momentan nicht. Ich überprüfe es ab und zu mal mit Valgrind. Momentan besitzt meine Klasse nur eine Membervariable. Eine Struktur, die in die Tiefe geht und an verschiedenen Stellen dynamischen Speicher verwendet. Mir ist nicht bekannt, ob man mit C++ eine einfache deep-copy anfertigen kann, daher muss ich das manuell machen.

viele Grüße,
SBond

Nathan

SBond schrieb:

Nathan schrieb:

Bei deiner copy() Funktion: wer gibt den alten Speicher frei? Zeig mal deine Member, ....

Der Speicher wird vom Destruktor freigegeben.

In diesem Code:

MyClass &MyClass::operator=(const MyClass &other)
{
    if (this != &other)
    {
        this->copy(*this, other);
    }
    return *this;
}

Enthält *this am Anfang dynamischen Speicher, wenn copy() keinen Speicher freigegeben wird, werden die Zeiger überschrieben. Es findet kein Destruktoraufruf statt, der alte Speicher wird also nie freigegeben nur der neue.
Das ist dann nicht viel besser als der default Assignment operator.

SBond

hätte nicht gedacht, dass mich das einfache kopieren von Objekten so fordert

hier ist mal eine vereinfachte Klasse, wie es bei mir ungefähr aussieht:

class MyClass : protected MyWrapper // MyWrapper besitzt selbst keine Membervariablen und stellt nur Methoden bereit
{
public:
	MyClass() : 
	mp_myStructure ((MyDeepStructure_t*) calloc(1, sizeof(MyDeepStructure_t))) // ja hier wird bewusst 'calloc' statt 'new' verwendet
	{
	}

	virtual ~MyClass() 
	{
		// gibt den dynamischen Speicher rekursiv frei (inklusive 'mp_myStructure' selbst)
		freeStructure(mp_myStructure);
	}

	//copy constructor
	MyClass(const MyClass &other) :
	mp_myStructure ((MyDeepStructure_t*) calloc(1, sizeof(MyDeepStructure_t)))
	{
		this->copy(*this, other);
	}

	//copy assignment
	MyClass &operator=(const MyClass &other)
	{
		this->copy(*this, other);
		return *this;
	}

	// ...weitere Klassenmethoden...

private:
	// interne Methoden
	void copy(MyClass &destination, const MyClass &source)
	{
		// kopiert die Inhalte von 'source.mp_myStructure->...' nach 'destination.mp_myStructure->...'
	}

	// Membervariablen (nur eine)
	MyDeepStructure_t *mp_myStructure;
};

Die Speicherverwaltung erfolgt mit calloc/free, da die externe Bibliothek die ich verwende auch damit arbeitet und ggf. ein realloc auf die Membervariable 'mp_myStructure' anwendet. Die Speicherfreigabe erfolgt auch über die externe Bibliothek, weshalb im Destruktor freeStructure() und nicht free() steht. Meine copy()-Methode erzeugt eine deep-copy von 'mp_myStructure'. Der Speicher für 'mp_myStructure' selbst, muss hierfür schon alloziert worden sein. Dies geschieht allerdings schon in den Konstruktoren. Wenn ich Kopie nach dem 'copy&swap'-Idiom machen möchte, wie würde dass denn hier aussehen? Ein std::swap mit 'mp_myStructure'? Bzw. wie könnte es anders aussehen?

viele Grüße,
SBond

dot

Um, was genau macht diese Bibliothek, dass da einfach so ein realloc() passieren kann? Wie willst du diese Deep-Copy genau anstellen, wenn du nichtmal weißt, was das eigentlich für eine Struktur ist, auf die dein Member zeigt?

SBond

dot schrieb:

Um, was genau macht diese Bibliothek, dass da einfach so ein realloc() passieren kann? Wie willst du diese Deep-Copy genau anstellen, wenn du nichtmal weißt, was das eigentlich für eine Struktur ist, auf die dein Member zeigt?

Es ist eine ASN.1-Bibliothek (asn1c). Die Struktur wird zuerst mit dem asn1c-Compiler generiert. Dieser erzeugt dann C-Code, welcher dann anschließend für die Implementierung genutzt wird. Die Inhalte der Struktur wird vor der Implementierung mit der ASN.1-Syntax definiert und daher ist mir auch der Aufbau der Struktur bekannt (etwas anderes hatte ich doch nicht behauptet). Der Zugriff und das Freigeben der einzelnen Elemente erfolgt über die Funktionsaufrufe der Bibliothek. Ich denke mal, dass der Inhalt in der Tiefe nicht so relevant ist.

Sollte es so viel Verwirrung schaffen, dann nehmen wir vereinfacht folgendes an:

typedef struct MyDeepStructure
{
	unsigned char *buf;
	int size;
} MyDeepStructure_t;

...und die Methode 'copy()' kopier die enthaltenen Daten 1:1 (alloziert für die Kopie neuen Speicher, kopiert den Inhalt von '*buf' und die Größe 'size')

dot

Nun, da ist mir aber weiterhin unklar, wo genau hier das struct selbst per realloc() modifiziert werden muss...

SBond

SBond schrieb:

Die Speicherverwaltung erfolgt mit calloc/free, da die externe Bibliothek die ich verwende auch damit arbeitet und ggf. ein realloc auf die Membervariable 'mp_myStructure' anwendet.

oh nein. Mein Fehler, sorry

Da habe ich es falsch formuliert. Das realloc wird auf die Elemente innerhalb der Membervariable 'mp_myStructure' anwendet und nicht auf die Membervariable selbst. 'mp_myStructure' wird allerdings durch eine Bibliotheksfunkton (mittels free()) freigegeben. Daher kommt kein new/delete zum Einsatz.

Sorry, dass ich hier was vertauscht habe.

dot

Bin immer noch etwas verwirrt: Wird mp_myStructure jetzt direkt per calloc()/free() erzeugt und freigegeben oder rufst du dazu irgendwelche Funktionen dieser Bibliothek auf, die lediglich intern calloc()/free() verwenden?

Torsten Robitzki

Meiner Meinung nach, macht Deine copy-Funktion genau dass, was operator= machen sollte:

class MyClass : protected MyWrapper // MyWrapper besitzt selbst keine Membervariablen und stellt nur Methoden bereit 
{ 
public: 
    MyClass() : 
        mp_myStructure ( static_cast< MyDeepStructure_t* >( calloc(1, sizeof(MyDeepStructure_t))))
    { 
    } 

    virtual ~MyClass() 
    { 
        // gibt den dynamischen Speicher rekursiv frei (inklusive 'mp_myStructure' selbst) 
        freeStructure(mp_myStructure); 
    } 

    MyClass(const MyClass &other) : 
        mp_myStructure ( static_cast< MyDeepStructure_t* >( calloc(1, sizeof(MyDeepStructure_t))))
    { 
        *this = other;
    } 

    MyClass &operator=(const MyClass &other) 
    { 
        // kopiert die Inhalte von 'other.mp_myStructure->...' nach 'this->mp_myStructure->...' 

        return *this; 
    } 

    // ...weitere Klassenmethoden... 

private: 
    // Membervariablen (nur eine) 
    MyDeepStructure_t *mp_myStructure; 
};

was aber eleganter wäre, wäre copy&swap:

class MyClass : protected MyWrapper // MyWrapper besitzt selbst keine Membervariablen und stellt nur Methoden bereit 
{ 
public: 
    MyClass() : 
        mp_myStructure ( static_cast< MyDeepStructure_t* >( calloc(1, sizeof(MyDeepStructure_t))))
    { 
    } 

    virtual ~MyClass() 
    { 
        // gibt den dynamischen Speicher rekursiv frei (inklusive 'mp_myStructure' selbst) 
        freeStructure(mp_myStructure); 
    } 

    MyClass(const MyClass &other) : 
        mp_myStructure ( static_cast< MyDeepStructure_t* >( calloc(1, sizeof(MyDeepStructure_t))))
    { 
        // kopiert die Inhalte von 'other.mp_myStructure->...' nach 'this->mp_myStructure->...' 
    } 

    MyClass &operator=(MyClass other) 
    { 
        swap( other );
        return *this; 
    } 

    void swap( MyClass& other )
    {
        std::swap( mp_myStructure, other.mp_myStructure );
    }    

private: 
    // Membervariablen (nur eine) 
    MyDeepStructure_t *mp_myStructure; 
};

HTH

SBond

dot schrieb:

Bin immer noch etwas verwirrt: Wird mp_myStructure jetzt direkt per calloc()/free() erzeugt und freigegeben oder rufst du dazu irgendwelche Funktionen dieser Bibliothek auf, die lediglich intern calloc()/free() verwenden?

Ja das ist in der Tat etwas verwirrend. Die Membervariable 'mp_myStructure' muss ich selber mit calloc() allozieren und initialisieren. Die Freigabe erfolgt allerdings über eine Bibliotheksfunktion. Die Bibliothek gibt dabei auch alle enthaltenen Elemente frei und das spart mir viel arbeit.

@Torsten: auch dir vielen Dank für die Unterstützung
Funktioniert prima.

Könnte man das nicht auch gleich so machen?:

MyClass &operator=(MyClass other) 
    { 
        std::swap( mp_myStructure, other.mp_myStructure );
        return *this; 
    }

Bzw. bringt es einen bestimmten Vorteil, swap() als Funktion auszulagern?

viele Grüße,
SBond

dot

SBond schrieb:

dot schrieb:

Bin immer noch etwas verwirrt: Wird mp_myStructure jetzt direkt per calloc()/free() erzeugt und freigegeben oder rufst du dazu irgendwelche Funktionen dieser Bibliothek auf, die lediglich intern calloc()/free() verwenden?

Ja das ist in der Tat etwas verwirrend. Die Membervariable 'mp_myStructure' muss ich selber mit calloc() allozieren und initialisieren. Die Freigabe erfolgt allerdings über eine Bibliotheksfunktion. Die Bibliothek gibt dabei auch alle enthaltenen Elemente frei und das spart mir viel arbeit.

Nur mal so aus Interesse: Was ist denn das bitte für eine Bibliothek?

SBond

dot schrieb:

Nur mal so aus Interesse: Was ist denn das bitte für eine Bibliothek?

Ein ASN.1-Compiler für C

http://lionet.info/asn1c/blog/

Wurde so wie es aussieht von einer einzelnen Person implementiert. Ist von der Dokumentation und Verwendung her etwas gewöhnungsbedürftig, aber ich bin froh dass ich es nutzen kann

Torsten Robitzki

SBond schrieb:

Könnte man das nicht auch gleich so machen?:

Ja, könnte man so machen. Wenn Du die swap() hast, dann kann sie aber auch von anderen genutzt werden. Du kannst auch std::swap() für deinen Typen überladen, und dann würden davon auch algorithmen profitieren, die intern swap() verwenden.

SBond

ok. vielen Dank