Smartpointer und move richtig verwendet?

Kerem

Ich Frage mich, ob ich bei dem unteren Beispiel das Prinzip von smartpointern und move richtig verstanden habe.

class Animal;

class Cage {
private:
   std::shared_ptr<Animal> m_animal;
public:
   Cage (std::shared_ptr<Animal> animal) : m_animal(std::move(animal)) {
      //do stuff with animal
   }  
   ~Cage() {
      //do stuff with animal
   }
};

class Collar {
   void Scan();
}

class Cat: public Animal {
   Collar collar;
}

class CatCage : public Cage {
private:
   std::shared_ptr<Cat> m_cat;
public:
   CatCage (std::shared_ptr<Cat> cat) : m_cat(cat) {
       m_cat.collar.Scan()
   }   
   ~CatCage () {
      //do stuff with cat
   }
};

//DLL-Schnittstelle
Cage* Create() {
   auto cat= std::make_unique<Cat>(); 
   return new CatCage(std::move(cat));
}
void Delete(Cage* cage) {
   delete cage;
}

Kann man das so machen oder seht ihr hier Risiken? Die DLL-Schnittstelle ist gegeben.

Bei der ursprünglichen Implementierung wurden raw pointer und new/delete verwendet. In Init-Methoden wurden dann die Member initialisiert. Zugriffe im Konstruktor gab es nicht, im Destruktor wurde auf NULL geprüft.

Jockelx

An smartptr und move ist erstmal nichts auszusetzen. Aber deine Klassenhierarchie ist überhaupt nicht gut:
Ein CatCage hat einen Zeiger auf eine Katze und auf ein Tier. Das scheint mir nicht so gewollt.

Kerem

Da der Zeiger auf Animal in Cage private ist, sollte doch CatCage nichts damit anfangen können? Im Speicher existiert nur ein Cat-Objekt. Cage und CatCage haben über den Zeiger die entsprechende Sicht darauf.

Edit

Ich habe Cat ein Collar spendiert, um die Problemstellung zu präzisieren. Vermutlich wäre es geschickter Collar nicht als Teil von Cat zu realisieren und separat an CatCage zu übergeben.

hustbaer

Welche DLL Schnittstelle? Und welche ursprüngliche Implementierung?

Und was soll das mit Cat und CatCage? Wieso nicht gleich Wurstbrot und Supermarkt? (Sorry, Insiderwitz)

Aber im Ernst: darunter kann man sich doch jetzt nichts vorstellen. Das Programm das du wirklich schreiben willst, wird sicher nicht Katzen und Katzen-Käfige modellieren. Und ohne zu wissen worum es wirklich geht und von was für einer DLL Schnittstelle du sprichst, können wir dir auch nicht sagen ob das so Sinn macht.

Was auffällt ist dass du shared_ptr und make_unique mischt. Das kann man so machen, kann auch in bestimmten Fällen Sinn machen. Normalerweise nimmt man aber eher make_shared wenn man shared_ptr verwendet.

Kerem

Danke für den Hinweis, tatsächlich ist es das erste mal, dass ich shared_ptr einsetze.

Ich modifiziere Klassen in einer bestehenden DLL so, dass kein new und delete mehr benötigt wird. Die DLL-Schnittstelle, über die man sich ein Objekt einer gewissen Basisklasse holen kann (oben wäre diese Cage, habe das Bsp. korrigiert), muss bleiben wie sie ist.

Das Beispiel ist wirklich nicht gelungen, aber es hat mir geholfen, das eigentliche Problem zu erkennen (danke @Jockelx ). Ich habe hier eine Art parallele Vererbungshierarchie vorliegen, die nicht zwingend notwendig ist und vermieden werden sollte.

Ansonsten gilt natürlich "Ein Wurstbrot ist ein Supermarkt!"

hustbaer

Oops, sorry, ich hatte das Kommentar "//DLL-Schnittstelle" in deinem Code übersehen.

Also grundsätzlich ja, das kannst du so machen. Ich würde wie gesagt das make_unique gegen make_shared tauschen. Oder den Zeigertyp in CatCage auf unique_ptr ändern. Muss aber nicht sein, funktionieren wird es auch so.

wob

Die andere Frage, die @Jockelx noch aufgeworfen hatte, war das

private:
   std::shared_ptr<Animal> m_animal;

in der Basisklasse und das dann folgende

private:
   std::shared_ptr<Cat> m_cat;

im Katzenkäfig.

Das ist so nicht sinnvoll, denn ein CatCage hat jetzt sowohl ein m_animal (wenn auch versteckt) als auch ein m_cat. Wenn du in der Cage-Klasse sowas hast wie string animal_name() const { return m_animal->name; } (natürlich unter der Annahme, dass ein Animal einen Namen hat), dann funktioniert das mit dem CatCage nicht mehr, weil dort das m_animal ja nicht gesetzt ist. Das ist also eine doofe Idee.

Lösungen können sein:

im CatCage einfach einen cast verwenden. Ist dann ein wenig "generic-style"
müssen die alle voneinander erben? Sonst einfach ein template Cage<T> machen?

Aber wieso hat das Cage überhaupt genau ein Tier drin? Können nicht vielleicht auch mehrere Tiere eingesperrt sein? Vielleicht sogar unterschiedliche Tiere? Ich finds nicht leicht, hier die richtige Lösung vorzuschlagen. Möglicherweise ist das Klassendesign noch nicht ganz durchdacht.

yahendrik

Tierbeispiele finde ich immer schlecht. Es soll jetzt kein Vorwurf an @Kerem sein, es ist leider verbreitet.
Was, wenn ein Animal irgendein Device ist, eine Katze ist ein Printer. Cage könnte irgendein Enumerator sein und ein Katzenkäfig könnte somit ein PrinterEnum sein.
Warum sollte nun ein PrinterEnum sowohl einen shared_ptr<Device> als auch einen shared_ptr<Printer> haben?
Wäre nicht irgendeine Liste (vector von Smartpointern zum Beispiel, siehe @wob ) angebrachter? Allerdings wäre ein PrinterEnum mit einem std::vector<Printer> mit Sicherheit viel angenehmer zu bedienen.
Ich glaube, dass der konkrete Fall sehr viel leichter verständlich für uns alle wäre.

Kerem

Das Beispiel sollte nur helfen die Fragestellung bezüglich der Pointer zu verdeutlichen. Ersetzt man Animal durch Device und Cage durch DeviceView entspricht es so ziemlich der eigentlichen Implementierung (@yahendrik warst nah dran :)).

Um nicht mehr von Katzen und Käfigen sprechen zu müssen (die bekommt da eh schwer rein^^):

class Device;

class DeviceView : public View {
private:
   std::shared_ptr<Device> m_device;
public:
   Cage (std::shared_ptr<Device> device) : m_device(std::move(device)) { }
};

class ScriptInterpreter{
   void RunScript(const std::string& script) { };
}

class DeviceGen1: public Device {
private:
   ScriptInterpreter m_scriptInterpreter;
public:
   ScriptInterpreter& ScriptInterpreter() {return m_scriptInterpreter;}
}

class DeviceGen1View: public DeviceView {
private:
   std::shared_ptr<DeviceGen1> m_deviceGen1; 

   OnRunButtonClick() {
     m_deviceGen1->RunScript(script);
   }

public:
   DeviceGen1View(std::shared_ptr<DeviceGen1> cat) : m_deviceGen1(device) { }     
};

//DLL-Schnittstelle
View* Create() {
   auto device = std::make_unique<DeviceGen1>(); 
   return new DeviceViewGen1(std::move(device));
}
void Delete(View* view) {
   delete view;
}

Es wird wohl darauf hinauslaufen für ScriptInterpreter ein eigenes View zu bauen.

wob

Das ist hier übrigens immer noch der gleiche Mist wie vorher, d.h. DeviceGen1View erbt von DeviceView und es gibt somit in dem DeviceGen1View einen std::shared_ptr<DeviceGen1> und einen (versteckten) std::shared_ptr<Device>. Das ist aus den schon genannten Gründen nicht sinnvoll.

Warum muss dein DeviceView überhaupt die konkrete (Kind-)Klasse des Device kennen?

Kerem

Der Mist wurde ja auch nur umbenannt...

Das konkrete Device verfügt über einen ScriptInterpreter, der über DeviceView bedient werden kann.

@Kerem sagte in Smartpointer und move richtig verwendet?:

Es wird wohl darauf hinauslaufen für ScriptInterpreter ein eigenes View zu bauen.

DocShoe

@Kerem
Über den Konstruktor von DeviceGen1View weisst du doch, dass m_device aus der Basisklasse nur ein DeviceGen1 sein kann. Damit kannst du m_device doch wieder in einen DeviceGen1 casten. Ist zwar nicht schön, aber was Besseres fällt mir auf die Schnelle auch nicht ein.