Copy-Konstruktor und operator=
-
Hallo,
ich habe eine Beispielklasse mit einem Copy-Konstruktor
und einem Zuweisungsoperator. Sind die Konstrukte Eurer
Meinung nach in Ordnung oder habt Ihr irgendwelche
Kommentare/Verbesserungsvorschläge?#include <map>
#include <list>
#include <string>class C {};
class B {};class A : public C {
public:
A();
~A();
A( const A& );
A& operator=( const A& );private:
struct str_type {
int a;
int b;
int *c;
} mStruct;std::map< std::string, int > mMap;
std::list< int > mList;
std::map< int, B* > mMap2;
static int mS;
const int mC;
B *mB; // Pointer to another class
};// Copy constructor
A::A( const A& a )
: mStruct( a.mStruct ),
mMap( a.mMap ),
mList( a.mList ),
mMap2( a.mMap2 ),
mC( a.mC ),
mB( new B( *a.mB ) ) {for (std::map< int, B* >::const_iterator it( a.mMap2.begin() );
it != a.mMap2.end(); ++it )
mMap2.insert( std::make_pair( it->first, new B( *(it->second) ) ) );
};// Assignment operator
A& A::operator=( const A& a ) {
if ( this != &a ) {
B* bakB = 0;
try {
bakB = new B( *a.mB );
}
catch( ... ) {
delete bakB;
throw;
}
C::operator=( a );
delete mB;
mB = bakB;
mStruct = a.mStruct;mMap = a.mMap;
mList = a.mList;
mMap2.clear();
for (std::map< int, B* >::const_iterator it( a.mMap2.begin() );
it != a.mMap2.end(); ++it )
mMap2.insert( std::make_pair( it->first, new B( *(it->second) ) ) );mS = a.mS;
}
return *this;
};Gruß,
Christian
-
verwende das nächste mal bitte cpp-tags. prinzipiell sieht das schon ganz gut aus, und du hast dir ja offenbar auch versucht, das ganze exception-sicher zu machen. das ist allerdings nicht wirklich gelungen, z.b. hier:
mB( new B( *a.mB ) ) { for (std::map< int, B* >::const_iterator it( a.mMap2.begin() ); it != a.mMap2.end(); ++it ) mMap2.insert( std::make_pair( it->first, new B( *(it->second) ) ) ); };in dieser schleife kännte es ohne weiteres passieren, dass eine exception geworfen wird. in diesem fall entsteht hier ein mehrfaches leck: da der konstruktor noch nicht beendet wurde, wird auch der destruktor von A nicht aufgerufen. damit ist das objekt, das durch mB referenziert wird, schon einmal verloren. und auch mMap2 weiß selbst nicht, wie seine elemente richtig freizugeben sind. generell ist es - aus diesem und aus anderen gründen - keine gute idee, stl-container mit normalen pointern zu füllen, sofern diese container die referenzierten objekte tatsächlich besitzen sollen (also logisch ihre lebenszeit kontrollieren). es ist nicht erkennbar, wieso mB bzw. mMap2 nicht direkt objekte vom typ B haben sollen. man könnte auch eine handle-/smartpointer-klasse für diesen zweck zu schreiben, das ist aber selten sinnvoll - es ist kein zufall, dass diese kategorie smart-pointer nicht in boost existiert. wenn mB bzw. mMap2 objekte statt zeiger benutzen, wird die definition eines eigenen copy-konstruktors und destruktors unnötig - die vom compiler implizt definierte version tut dann genau das richtige (und dass möglichlicherweise effizienter). es könnte immer noch sinnvoll sein, den copy-zuweisungsoperator selbst zu definieren (etwa per copy&swap), denn der implizit definierte operator gib keinerlei garantien hinsichtlich exception-sicherheit.
-
Danke für Deine ausführliche Antwort.
Aus Deiner Antwort entnehme ich, dass es keine einfache und sichere Methode
gibt, um Copy-Konstruktoren zu schreiben, die das Klonen von STL maps, welche
wiederrum Pointer enthalten (angenommen es geht nicht anders), regeln. Denn
das Problem bleibt, dass ein "new" fehlschlägt und das das erzeugende Objekt
inkonsistent wird.Wie wird das Problem sonst in der Praxis gelöst? Einfach ignorieren und hoffen,
dass es gut geht?
-
Um nochmal auf das Beispiel zurückzukommen, das Du aus meinem Code herausgepickt
hast:
Wäre es denn zumindest eine kleine Hilfe, dass man in der Schleife eine
weitere Ausnahme-Behandlung einbaut, die zumindest den User bei einem
fehlgeschlagenen "new" informiert, also etwa so:for (std::map< int, B* >::const_iterator it( a.mMap2.begin() ); it != a.mMap2.end(); ++it ) { try { mMap2.insert( std::make_pair( it->first, new B( *(it->second) ) ) ); } catch( ... ) { throw; } }Ich war mir des Problems mit der dynamischen Erzeugung von Objekten bisher
nicht wirklich so bewußt, aber müßte man das sicherheitshalber nicht alle
"new"-Operatoren in einen try{}-Block legen, um Fehler abzufangen?
-
zubrowa schrieb:
Ich war mir des Problems mit der dynamischen Erzeugung von Objekten bisher
nicht wirklich so bewußt, aber müßte man das sicherheitshalber nicht alle
"new"-Operatoren in einen try{}-Block legen, um Fehler abzufangen?das problem ist nicht new selbst, sondern jede operation, die eine exceptionwerfen könnte, wenn gleichzeitig resourcen in 'dummen' handles - hier normale pointer - gehalten werden. immer dann wäre an sich ein try/catch notwendig. und genau hier setzt RAII an: die resource wird ein objekt gebunden, dass selbst in der lage ist, diese resource freizugeben, wenn das objekt zerstört wird (auf normalem weg oder während des stack-unwindings) - der bakannteste vertreter dieser zunft ist eben der smart-pointer. im allgemeinen kann man sagen, dass exception-sicherheit nichts mit try und catch zu tun hat (bzw. haben sollte in sauberem code).
zubrowa schrieb:
Um nochmal auf das Beispiel zurückzukommen, das Du aus meinem Code herausgepickt
hast:
Wäre es denn zumindest eine kleine Hilfe, dass man in der Schleife eine
weitere Ausnahme-Behandlung einbaut, die zumindest den User bei einem
fehlgeschlagenen "new" informiert, also etwa so:for (std::map< int, B* >::const_iterator it( a.mMap2.begin() ); it != a.mMap2.end(); ++it ) { try { mMap2.insert( std::make_pair( it->first, new B( *(it->second) ) ) ); } catch( ... ) { throw; } }tatsächlich könntest du das leck hier sogar komplett schliessen - grundsätzlich ist es möglich, mittels try/catch exception-sicherheit herzustellen, im allgemeinen ist das ergebnis allerdings extrem unwartbarer code (vermischen von normalem code und exception-handling widerspricht dem grundgedanken dahinter):
// Copy constructor A::A( const A& a ) : mStruct( a.mStruct ), mMap( a.mMap ), mList( a.mList ), mMap2( a.mMap2 ), mC( a.mC ), mB( new B( *a.mB ) ) { try { for (std::map< int, B* >::const_iterator it( a.mMap2.begin() ); it != a.mMap2.end(); ++it ) { B* p = new B( *(it->second) ); // ein new-ausdruck als funktionsargument führt sehr schnell zu einem leck - hier, wenn new gelingt, aber insert fehlschlägt try { mMap2.insert( std::make_pair( it->first, p ) ); } catch ( ... ) { delete p; throw; } } } catch ( ... ) { for (std::map< int, B* >::iterator it = mMap2.begin(); it != mMap2.end(); ++it ) delete it->second; delete mB; throw; } };(ohne garantie) - das ist noch ein recht einfacher fall und trotzdem schon furchtbar. es heisst jha eigentlich auch nicht umsonst exception-handling: wir wollen uns mit der exception selbst beschäftigen und nicht mit nebensächlichkeiten wie hinter schlecht geschriebenem code aufzuräumen.
edit: das innere try-catch wäre hier noch relativ leicht mit standardmitteln zu eliminieren:
try { for (std::map< int, B* >::const_iterator it( a.mMap2.begin() ); it != a.mMap2.end(); ++it ) { std::auto_ptr< B > p( new B( *(it->second) ) ); mMap2.insert( std::make_pair( it->first, p.get() ) ); p.release(); // wird nicht erreicht, wenn insert wirft } } catch ( ... ) { for (std::map< int, B* >::iterator it = mMap2.begin(); it != mMap2.end(); ++it ) delete it->second; delete mB; throw; }sehr überzeugend ist das trotzdem nicht. eine weitere alternative wäre die verwendung von boost:ptr_map. ob das sinnvoll ist, hängt primär vom grund ab, aus dem nicht B sondern B* jeweils als member benutzt wird.
-
Mir viel noch gerade auf, dass die struct mStruct einen int pointer enthält,
daher kann man das Kopieren/Zuweisen nicht mit einer shallow copy regeln,
sondern muß eine deep copy durchführen, also in etwas so:A::A( const A& a)
: ...
{
mStruct.a = a.mStruct.a;
mStruct.b = a.mStruct.b;
mStruct.c = new int( *a.mStruct.c );
...
}Muß denn dann mStruct.c vom Destruktor mit "delete" gelöscht werden?
Vermutlich hängt es davon ab, wie der Konstruktor aussieht.