unique_ptr aus map auslesen und wo ist make_unique?

[temi]

Folgendes Problem. Ich habe in "find" alle drei Varianten reingeschrieben. Ich möchte Variante 2, um einen Nullptr zurückzugeben, wenn die Id nicht existiert.

template<class T> class Catalog
{
    public:
        Catalog() {};
        void add(T* object);
        // noch mehr Kram
    private:
        std::unordered_map<ulong, std::unique_ptr<T>> map;
};

template<class T> void Catalog<T>::add(T* object)
{
    map.insert(std::make_pair<ulong, std::unique_ptr<T>>(dynamic_cast<Object*>(object)->getId(), std::unique_ptr<T>(object)));
}

template<class T> T* Catalog<T>::find(ulong id) const
{
    return map.at(id).get(); // 1. dies funktioniert

    auto it = map.at(id); // 2. das funktioniert nicht!
    return it != map.end() ? it.get() : nullptr; // soweit komme ich gar nicht

    return map[id].get(); // 3. das funktioniert auch nicht
}

Das ist der Fehler bei 2.:

Fehler: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = tad::Item; _Dp = std::default_deletetad::Item]'
auto it = map.at(id);

Das ist der Fehler bei 3.:

Fehler: passing 'const std::unordered_map<long unsigned int, std::unique_ptr<tad::Item, std::default_deletetad::Item >, std::hash<long unsigned int>, std::equal_to<long unsigned int>, std::allocator<std::pair<const long unsigned int, std::unique_ptr<tad::Item, std::default_deletetad::Item > > > >' as 'this' argument discards qualifiers [-fpermissive]
return map[id].get()

Was geht da vor?

Fehlt bei der Item-Klasse irgendwas?

Kann mir außerdem mal jemand sagen wo ich make_unique finde? Ich habe <memory> eingebunden. std::make_shared funktioniert, std::make_unique nicht.

temi

[zufallswert]

make_unique gibt's ab C++14. Vermißt man es in C++11, kann man es leicht selber implementieren.

[wob]

Ich finde den Code sehr fragwürdig.

1. Du nimmst einen Raw-Pointer als Parameter an und machst dann einen unique_ptr daraus. D.h. du hast ein Sink, das es nicht nach draußen zeigt.

2. Was soll das dynamic_cast hier? Gehört da bestimmt nicht hin!

auto it = map.at(id);
return it != map.end() ? it.get() : nullptr; // soweit komme ich gar nicht

Das ist ja auch Blödsinn. map.at liefert dir eine Referenz auf das Objekt, keinen Iterator! Daher ist erstens der Name it unglücklich gewählt. Zweitens ist dein value-type der Tap ein unique_ptr. Den kann man nicht kopieren (logischerweise, denn dann wäre er ja nicht unique), sondern nur verschieben (oder man holt sich eben mit .get den Raw-Pointer). Wolltest du stattdessen vielleicht "map.find(...)" schreiben?

4. Was ist eigentlich der große Sinn der Klasse Catalog? ISt der irgendwie mehr als nur diese Map? Soll der Catalog wirklich immer Eigentümer der Objekte sein?

Ich habe aber das Gefühl, dass dir move-Semantik und auch die Smart-Pointer noch nicht ganz klar sind, weiß aber nicht so recht, wie ich da am besten helfen kann.

Zu deinem Spiel: Was passiert in Fällen wie "benutzt Axt mit Baum", wo danach der Baum weg ist und stattdessen Kleinholz da sind? Wem gehört dann was? Und wer erzeugt eigentlich deine Objekte wie Axt und Schleifpapier?

[temi (temp)]

wob schrieb:

1. Du nimmst einen Raw-Pointer als Parameter an und machst dann einen unique_ptr daraus. D.h. du hast ein Sink, das es nicht nach draußen zeigt.

Stimmt. Ich könnte auch ein "Catalog.Create(TypeToCreate)" schreiben, aber dann wäre der Catalog gleichzeitig auch eine Fabrik und das wollte ich eigentlich nicht. Separation of concerns. Möglich wäre allerdings auch die Übergabe eines
unique_ptr.

wob schrieb:

2. Was soll das dynamic_cast hier? Gehört da bestimmt nicht hin!.

Der Katalog darf nur Objekte aufnehmen, die von "Object" abgeleitet sind (eigentlich IObject, weil idealerweise auf ein Interface programmiert wird) UND benötigt "getId()" (Interface!), um vom übergebenen Objekt die Id auslesen zu können. Bei der Deklaration eines Templates kann ich ja nicht angeben, was "T" sein darf und was nicht. Bei meiner Lösung wird auch korrekt zur Kompilierzeit ein Fehler geworfen, wenn ein falsches Objekt gespeichert werden soll.

wob schrieb:

3. map.at liefert dir eine Referenz auf das Objekt, keinen Iterator! Daher ist erstens der Name it unglücklich gewählt. Zweitens ist dein value-type der Tap ein unique_ptr. Den kann man nicht kopieren (logischerweise, denn dann wäre er ja nicht unique), sondern nur verschieben (oder man holt sich eben mit .get den Raw-Pointer). Wolltest du stattdessen vielleicht "map.find(...)" schreiben?

Da hast du recht, es ist kein Iterator. War wohl gestern zu spät. Bei Variante1 steht auch das .get() und bei Variante 2 ebenfalls, denn genau diesen Zeiger möchte ich auch haben. Allerdings wird der Fehler ja bereits in der Zeile "auto it = map.at(id);" geworfen und zwar unabhängig vom Name "it".

wob schrieb:

4. Was ist eigentlich der große Sinn der Klasse Catalog? ISt der irgendwie mehr als nur diese Map? Soll der Catalog wirklich immer Eigentümer der Objekte sein?

Der Katalog enthält ganz einfach alle erzeugten Objekte und bei Bedarf kann ein Zeiger darauf abgerufen werden. Damit ist der Katalog der Eigentümer und kümmert sich um die Speicherverwaltung und alle anderen verwenden nur Zeiger auf die verwalteten Objekte. Ist die Idee so dumm? Nebenbei wäre das für einen hypothetischen "Gameeditor" einfach, weil so eine Liste aller bereits erstellten Objekte zum Auswählen bereit steht.

wob schrieb:

Ich habe aber das Gefühl, dass dir move-Semantik und auch die Smart-Pointer noch nicht ganz klar sind, weiß aber nicht so recht, wie ich da am besten helfen kann.

Damit könntest du recht haben. Wie schon geschrieben, ich komme von C#, da muss man sich darum nicht so sehr kümmern. Ich versuche mich da durch Lesen und Probieren einzufinden. Das Problem ist, dass vieles funktioniert, aber trotzdem ungünstig oder gefährlich sein kann.

wob schrieb:

Zu deinem Spiel: Was passiert in Fällen wie "benutzt Axt mit Baum", wo danach der Baum weg ist und stattdessen Kleinholz da sind? Wem gehört dann was? Und wer erzeugt eigentlich deine Objekte wie Axt und Schleifpapier?

Die Objekte sollen aus einer Datei/DB erzeugt werden. Der "Spieler" hat in seinem "Inventar" das "Item" "Axt", befindet sich in der "Location", z.B. "Wald". Die "Location" enthält das "Item" "Baum" (benutzbares Item) und enthält anschließend noch das "Item" "Kleinholz". "Baum" könnte jetzt aus "Wald" entfernt werden oder über ein Flag als "bentzt" gekennzeichnet werden.

Aber das "Spiel" ist nur Mittel zum Zweck mit C++ warm zu werden. Da es sich um ein "Textspiel" handelt muss ich mich noch nicht mit irgendwelchen GUIs herumschlagen.

Vielen Dank für deine bisherige Unterstützung bei meinen Bemühungen,
temi

[wob]

temi (temp) schrieb:

wob schrieb:

2. Was soll das dynamic_cast hier? Gehört da bestimmt nicht hin!.

Der Katalog darf nur Objekte aufnehmen, die von "Object" abgeleitet sind (eigentlich IObject, weil idealerweise auf ein Interface programmiert wird) UND benötigt "getId()" (Interface!), um vom übergebenen Objekt die Id auslesen zu können.

Jetzt gibt es natürlich 2 Fragen:
1. Warum ist es überhaupt ein Template - du könntest ja einfach T durch Object oder IObject ersetzen und fertig. Unter der Annahme, dass es getId() in IObject schon gibt. Templates brauchst du, wenn du jeweils einen Katalog für Object, einen für Tiere, einen für Menschen etc. haben willst (die alle nichts miteinander zu tun haben). Du hast aber hier nur Object.
2. Wenn du ein Template nimmst, dann könnte man mit deinem Code einen Kalalog von int machen und zur Laufzeit(!) bekommst du dann eine Nullpointer-Dereferenzierung, weil int* in Object* gedynamiccastet wird und das erbigt nullptr). Das ist also nicht gut so! Zur Kompilezeit kannst du mittels static_assert(is_base_of<Base, Derived>::value, ""); testen, ob der erwartete Typ im Template ist. Ich vermute aber mal, dass du hier vielleicht gar kein Template brauchst.

Allerdings wird der Fehler ja bereits in der Zeile "auto it = map.at(id);" geworfen und zwar unabhängig vom Name "it".

Weil du unique_ptr nicht kopieren kannst! Dann hättest du ja 2 unique_ptr auf dasselbe Objekt, was ja Blödsinn wäre.

[Temi (temp)]

wob schrieb:

Jetzt gibt es natürlich 2 Fragen:
1. Warum ist es überhaupt ein Template - du könntest ja einfach T durch Object oder IObject ersetzen und fertig. Unter der Annahme, dass es getId() in IObject schon gibt. Templates brauchst du, wenn du jeweils einen Katalog für Object, einen für Tiere, einen für Menschen etc. haben willst (die alle nichts miteinander zu tun haben). Du hast aber hier nur Object.
2. Wenn du ein Template nimmst, dann könnte man mit deinem Code einen Kalalog von int machen und zur Laufzeit(!) bekommst du dann eine Nullpointer-Dereferenzierung, weil int* in Object* gedynamiccastet wird und das erbigt nullptr). Das ist also nicht gut so! Zur Kompilezeit kannst du mittels static_assert(is_base_of<Base, Derived>::value, ""); testen, ob der erwartete Typ im Template ist. Ich vermute aber mal, dass du hier vielleicht gar kein Template brauchst.

Da hast du Recht, aber dann müsste ich die daraus ausgelesenen Zeigertypen (Item, Location, Person, ...) jeweils auf den korrekten Typen casten. Bei der generischen Variante entfällt das. Die Sache mit dem "assert" schau ich mir trotzdem mal an.

wob schrieb:

Weil du unique_ptr nicht kopieren kannst! Dann hättest du ja 2 unique_ptr auf dasselbe Objekt, was ja Blödsinn wäre.

Danke, ich bin blöd...

[wob]

Temi (temp) schrieb:

Da hast du Recht, aber dann müsste ich die daraus ausgelesenen Zeigertypen (Item, Location, Person, ...) jeweils auf den korrekten Typen casten. Bei der generischen Variante entfällt das.

Aber du hast doch trotzdem in deinem Code einen dynamic_cast drin! Warum? Du sagst doch gerade, das sei jetzt generisch.

[temi (temp)]

wob schrieb:

Temi (temp) schrieb:

Da hast du Recht, aber dann müsste ich die daraus ausgelesenen Zeigertypen (Item, Location, Person, ...) jeweils auf den korrekten Typen casten. Bei der generischen Variante entfällt das.

Aber du hast doch trotzdem in deinem Code einen dynamic_cast drin! Warum? Du sagst doch gerade, das sei jetzt generisch.

Gibt es eine andere Möglichkeit?

Ich möchte dem Catalog<T> Objektzeiger auf Objekte übergeben, die die Schnittstelle IObject (oder genauer IHasId) implementieren und beim Abrufen den gespeicherten Typzeiger zurück erhalten => T* find(ulong id).

Natürlich könnte ich sowas machen: void add(ulong id, IHasId object). Damit braucht der Katalog von dem Zugriff auf die Id nichts zu wissen, ABER das muss doch auch in C++ machbar sein. In C# ist das einfach:

public class Catalog<T> where T : IHasId

[Th69]

Wie wob schon schrieb, du brauchst hier überhaupt kein Template:

class Catalog
{
    public:
        Catalog() {};
        void add(IHasId* object);
        // noch mehr Kram
    private:
        std::unordered_map<ulong, std::unique_ptr<IHasId>> map;
};

Und selbst in deiner Template-Variante reicht ein:

object->getId() // ohne cast!

Jede Klasse, welche dann eine

ulong getId()

bietet, kann dann benutzt werden.

C++ templates sind unabhängig von irgend einer Klassenhierarchie.

[firefly]

Th69 schrieb:

C++ templates sind unabhängig von irgend einer Klassenhierarchie.

Hier bist du einem missverständnis aufgesessen.
Die where clause in einem C# Generic bewirkt eine Einschränkung mit welchen Typen das Generic verwendet werden kann. (generic type constraint)
In dem beispiel von Temi (temp)

public class Catalog<T> where T : IHasId

kann die Generic klasse nur verwendet werden wenn "T" von Typ IHashId ist bzw. eine von dieser klasse/interface abgeleiteter Typ ist

[temi]

Ich würde gerne den Typ rausbekommen, den ich reingesteckt habe, also:

class Item : public IHasId, public IItem
{
//..
}

class Location : public IHasId, public ILocation
{
//..
}

itemCatalog.add(new Item);
locationCatalog.add(new Location);

item* i = itemCatalog.find(id);

location* l = locationCatalog.find(42);

Von einer Klasse die nur "IHasId" aufnimmt, bekomme ich auch nur IHasId zurück, oder etwa nicht?

Angenehm wäre dabei vielleicht, dass nur ein Katalog alle Typen aufnehmen könnte, aber dann müsste ich nachdem ich meinen Zeiger zurück habe casten.

ILocation* l = dynamic_cast<ILocation*>(completeCatalog.find(42))

Das ist aber ziemlich unsicher, weil ich ja u.U. nicht genau weiß, was hinter der Id "42" gelistet ist. Dazu bräuchte ich dann zumindest ein (C#):

if (completeCatalog.find(42) is ILocation)

Soweit ich gesehen habe geht das aber bei C++ nicht, oder?

Eine Möglichkeit wäre noch IHasId um eine Angabe des Types a la "enum ObjectType..." zu erweitern, um den korrekten Typ zu ermitteln.

temi und temi (temp) bin übrigens beides ich, auf der Arbeit hatte ich meine Anmeldedaten nicht parat.

Danke für die anregende Diskussion,
temi

[temi]

Mir fällt gerade ein, ich könnte natürlich auch für jeden Typen eine eigene Suchmethode anbieten:

class Catalog
{
    public:
        Catalog() {};
        void add(IHasId* object);
        // noch mehr Kram
        IItem* findItem(ulong id);
        ILocation* findLocation(ulong id);
        // usw. :) 
    private:
        std::unordered_map<ulong, std::unique_ptr<IHasId>> map;
};

So richtig hübsch ist das aber auch nicht.

Überladen einer Methode mit unterschiedlichen Rückgabewerten in der Signatur ist soweit ich gelesen habe ja nicht möglich?

Aber das müsste sich doch irgendwie machen lassen, oder?

auto l = catalog.find(42).asLocation(); // Typprüfung intern!?

[temi]

wob schrieb:

2. Wenn du ein Template nimmst, dann könnte man mit deinem Code einen Katalog von int machen und zur Laufzeit(!) bekommst du dann eine Nullpointer-Dereferenzierung, weil int* in Object* gedynamiccastet wird und das ergibt nullptr).

Hab ich grad ausprobiert, das kompiliert nicht.

Catalog<int> c2;
c2.add(new int{42});

Fehler: cannot dynamic_cast 'object' (of type 'int*') to type 'class tad::Object*' (source is not a pointer to class)
map.insert(std::make_pair<ulong, std::unique_ptr<T>>(dynamic_cast<Object*>(object)->getId(), std::unique_ptr<T>(object)));

[Th69]

firefly schrieb:

Th69 schrieb:

C++ templates sind unabhängig von irgend einer Klassenhierarchie.

Hier bist du einem missverständnis aufgesessen.
...

Hä??? Ich weiß, wie C#-Generics funktionieren - und ich schrieb, daß eben bei den C++ Templates der zu verwendende Typ nicht von irgendeiner Klassenhierarchie (wie eben bei den C# Generics) abhängig ist.

Und @temi:

template<class T> T* Catalog<T>::find(ulong id) const
{
    return map.at(id).get(); // 1. dies funktioniert

    auto const &it = map.at(id); // 2. so funktioniert das!
    return it != map.end() ? it.get() : nullptr;

    return map[id].get(); // 3. das funktioniert auch nicht - kann auch nicht, denn dies funktioniert nur bei nichtkonstanten Funktionen
                          // (denn bei Bedarf wird hier ein Element der map hinzugefügt).
}

[firefly]

Th69 schrieb:

firefly schrieb:

Th69 schrieb:

C++ templates sind unabhängig von irgend einer Klassenhierarchie.

Hier bist du einem missverständnis aufgesessen.
...

Hä??? Ich weiß, wie C#-Generics funktionieren - und ich schrieb, daß eben bei den C++ Templates der zu verwendende Typ nicht von irgendeiner Klassenhierarchie (wie eben bei den C# Generics) abhängig ist.

Deine Antwort klang aber anders, sorry wenn ich dich falsch verstanden haben sollte

[temi]

Ich hab es jetzt so gemacht. Damit ist der Iterator auch wirklich ein Iterator. Danke an wob für die Erleuchtung.

template<class T> T* Catalog<T>::find(ulong id) const
{
    auto it = map.find(id);
    return it != map.end() ? it->second.get() : nullptr;
}

Ich würde mich dennoch über Antworten, meine weiteren Überlegungen betreffend, freuen (siehe oben).

Speziell dieses würde mich recht reizen.

auto l = catalog.find(42).asLocation();

[wob]

temi schrieb:

Hab ich grad ausprobiert, das kompiliert nicht.

Ok, da habe ich zu viel vereinfacht. Es braucht schon einen Klassentypen. Probiere doch statt int mal
struct Foo { int i };
aus und setzte new Foo statt new int ein.

[temi]

wob schrieb:

temi schrieb:

Hab ich grad ausprobiert, das kompiliert nicht.

Ok, da habe ich zu viel vereinfacht. Es braucht schon einen Klassentypen. Probiere doch statt int mal
struct Foo { int i };
aus und setzte new Foo statt new int ein.

Fehler: cannot dynamic_cast 'object' (of type 'struct Foo*') to type 'class tad::Object*' (source type is not polymorphic)
map.insert(std::make_pair<ulong, std::unique_ptr<T>>(dynamic_cast<Object*>(object)->getId(), std::unique_ptr<T>(object)));

[wob]

temi schrieb:

Fehler: cannot dynamic_cast 'object' (of type 'struct Foo*') to type 'class tad::Object*' (source type is not polymorphic)
map.insert(std::make_pair<ulong, std::unique_ptr<T>>(dynamic_cast<Object*>(object)->getId(), std::unique_ptr<T>(object)));

Dann spendiere dem Foo halt noch eine virtuelle Funktion!

[temi]

wob schrieb:

temi schrieb:

Fehler: cannot dynamic_cast 'object' (of type 'struct Foo*') to type 'class tad::Object*' (source type is not polymorphic)
map.insert(std::make_pair<ulong, std::unique_ptr<T>>(dynamic_cast<Object*>(object)->getId(), std::unique_ptr<T>(object)));

Dann spendiere dem Foo halt noch eine virtuelle Funktion!

Ich gebe auf.

Tad3 ist abgestürzt

static_assert(std::is_base_of<Object, T>::value, "Catalog<T>: T is not derived from Object");

Hat geklappt.