Einmal bitte über eine kleine Klasse drüberschauen (Allocate memory)



  • Habe eine kleine Klasse definiert, die entweder raw memory mittels eines allocator objects anlegt oder aber nen pointer zu Daten übergeben bekommt.

    habt ihr Kritik, Verbesserungsvorschläge oder Ähnliches? 🙂

    #pragma once
    #include <iostream>
    #include <memory>
    #include <string>
    
    class Memory {
     public:
      explicit Memory(std::string s = "Memory Constructor called") {
        std::cout << s;
      }
      virtual ~Memory() {
        if (_maxDataSize)
          _alloc.deallocate(_dPtr.get(), _maxDataSize);
      };
    
      void setInternalMem(uint32_t maxDataSize) {
        if (!_maxDataSize) {  // only if memory is not yet allocated
          _dPtr.reset(_alloc.allocate(maxDataSize));
          _maxDataSize = maxDataSize;
        }
      }
    
      void setData(std::shared_ptr<uint8_t> sPtr) {
        if (_maxDataSize) {  // deallocate requires a built-in pointer
          _alloc.deallocate(_dPtr.get(), _maxDataSize);
          _maxDataSize = 0;
        }
        _dPtr = sPtr;  // reset the pointer to the passed content
      }
    
      std::shared_ptr<uint8_t> getData() {
        return {_dPtr};
      }
    
     private:
      std::allocator<uint8_t> _alloc{};  // default-initialize allocator object
      std::shared_ptr<uint8_t> _dPtr{};  // defaults to nullptr anyways
      uint32_t _maxDataSize{0};
    }
    

  • Mod

    • Der Default-Konstruktor ist einfach blödsinnig.
    • uint32_t statt size_t (und uint8_t statt unsigned char ? Oder ist das bewusst so gemacht?) ist eine fragliche Wahl der Typen.
    • Sowohl der Destruktor als auch setData geben Speicher potenziell doppelt frei. Und setData setzt _maxDataSize auf Null, was natürlich inkorrekt ist, wenn das Argument nicht selbst Null ist. (Ironischerweise löst dies das Problem mit der doppelten Freigabe...)
    • !_maxDataSize ist ein zu Recht verpönter Weg, eine Zahl auf Null zu prüfen, und vice versa für _maxDataSize . Schreib doch einfach _maxDataSize == 0 .

    Da ist noch mehr, aber ich habe auch Schwierigkeiten mir die intendierte Anwendung vorzustellen. Wenn du ein paar use cases demonstrierst, können wir dir womöglich auch ein sauberes Design vorschlagen.



  • sieht ein wenig komisch aus - relativ viel Code für das bisschen Funktionalität, warum nutzt du den std::allocator wenn du ihn sowieso nicht verändern kannst - new wäre da nicht weniger "crackig"



  • Vielen Dank für das Feedback. Es geht um Datenverarbeitung im automobilbereich, das hier ist nur die Basisklasse für weitere Klassen, die das ganze Spezifizieren.

    - Wieso ist der Default Ctor blöd? der dient mir halt nur zum debuggen

    - ich dachte, durch die Wahl von uint32_t usw. mache ich das ganze Systemunabhängig? wieso also fraglich?

    - wieso wird Speicher doppelt frei gegeben?

    - Wieso ist der vergleich verpöhnt?

    Danke nochmal


  • Mod

    Sewing schrieb:

    - Wieso ist der Default Ctor blöd? der dient mir halt nur zum debuggen

    Der dient vielleicht dir zum debuggen (wovon ich dir natürlich ebenfalls abrate), aber für alle anderen Entwickler ist das überflüssiger Müll der in stdout reingespammt wird. Stell dir mal vor, jede Klasse kündigt sich so an, das wäre lachhaft.

    - ich dachte, durch die Wahl von uint32_t usw. mache ich das ganze Systemunabhängig? wieso also fraglich?

    Weil size_t gerade dafür konzipiert ist, Speichermengen anzugeben, und deswegen gerade am platformunabhängisten ist. Auf Maschinen, auf denen sich mehr als UINT32_MAX allozieren lässt, hast du somit eine Barriere eingeführt.

    - wieso wird Speicher doppelt frei gegeben?

    Weil shared_ptr RAII konform seinen Pointee stets freigibt. Und du diese Aufgabe noch einmal selbstständig erfüllst.

    - Wieso ist der vergleich verpöhnt?

    Weil er nicht das aussagt, was der Code tut. Deine Syntax suggeriert, dass der Operand ein bool ist. Jemand, der den Code, aus welchem Grund auch immer, ohne genug Kontext zu verstehen versucht, wird dadurch womöglich verwirrt.



  • Sewing schrieb:

    habt ihr Kritik, Verbesserungsvorschläge oder Ähnliches? 🙂

    Ja, hab ich. Ich hab' z.B. keinen Plan was die Klasse eigentlich machen soll. Der Sinn der Funktionen setInternalMem() und setData() erschliesst sich mir nicht. Und wenn das ganze als Basisklasse gedacht ist, finde ich es fragwürdig dass die ganzen Memberfunktionen public sind.

    Der Kritik von Arcoth kann ich mich mit einer klitzekleinen Ausnahme ebenfalls anschliessen. Und diese ist: ich persönliche finde if (!integerVariable) nicht schlimm. Ist aber auch Gewohnheitssache. Und grundsätzlich hat er natürlich Recht dass if (integerVariable == 0) "deutlicher" ausdrückt was da gemacht wird.



  • Es geht um das Speichern der Daten von Sensoren. Diese Basisklasse ist allen Sensortypen gemein und wird dann eben durch Vererbung noch weiter spezifiziert. Aber generell sollen auch die Subklassen eben diese member funktionen unverändert haben.

    Es soll grundsätzlich möglich sein, mittels setInternalMem unconstructed Speicher auf dem Heap zu allozieren. Könnte das auch mit new uint8_t[_maxSize] machen, wollte das aber mal mit std::allocastor probieren. Ist das falsch?

    Dann soll die Klasse aber alternativ auch die Möglichkeit bieten, durch setData einen Pointer auf einen bestehenden Datenbereich zu setzen.

    - size_t ist doch gleichbedeutend mit uint16_t oder? was wenn mehr als 16bit integer gefragt sind?

    - wann verwende ich allgemein size_t und wann beim iterieren über einen container bspw. using sizet = std::vector::size_type? Wo besteht da der Unterschied?

    - Ich verstehe nicht, weshalb in setData der Speicher zweimal freigegeben wird. Wenn die if als true evaluiert, dann ist bereits Speicher alloziert worden, und den muss ich doch freigeben, da hier der shared_pointer doch nicht destroyed wird, oder wie?


  • Mod

    😮 was bist du bloß für ein Heuchler! Du warst derjenige, der mich vor Jahren deswegen mal kritisiert hat. Daher habe ich überhaupt diese Überzeugung. Oder war das vielleicht Bashar?



  • Arcoth schrieb:

    😮 was bist du bloß für ein Heuchler! Du warst derjenige, der mich vor Jahren deswegen mal kritisiert hat. Daher habe ich überhaupt diese Überzeugung. Oder war das vielleicht Bashar?

    Das wäre jetzt interessant 🙂
    Keine Ahnung ob ich dich mal diesbezüglich kritisiert habe.
    Möglich dass ich das vor etlichen Jahren anders gesehen habe - man ändert ja auch manchmal seine Meinung. So genau weiss ich nimmer was ich vor vielen Jahren für gut bzw. schlecht gehalten habe 😉

    Und ich hab ja gesagt: grundsätzlich hast du schon Recht, die size == 0 Variante ist vermutlich besser.



  • Sewing schrieb:

    Es soll grundsätzlich möglich sein, mittels setInternalMem unconstructed Speicher auf dem Heap zu allozieren. Könnte das auch mit new uint8_t[_maxSize] machen, wollte das aber mal mit std::allocastor probieren. Ist das falsch?

    Was ist schon falsch?
    Die Frage ist eher: Ist es üblich/verständlich? Und die Antwort ist "nein und nein".
    Daher die nächste Frage: was versprichst du dir davon?
    Und wofür brauchen die Klassen überhaupt nicht-initialisierten Speicher?

    Sewing schrieb:

    Dann soll die Klasse aber alternativ auch die Möglichkeit bieten, durch setData einen Pointer auf einen bestehenden Datenbereich zu setzen.

    Wozu?
    Kannst du ein (konkretes!) Anwendungsbeispiel bringen?

    Sewing schrieb:

    - size_t ist doch gleichbedeutend mit uint16_t oder? was wenn mehr als 16bit integer gefragt sind?

    Wie kommst du denn auf die Idee?
    http://en.cppreference.com/w/c/types/size_t

    size_t can store the maximum size of a theoretically possible object of any type (including array).

    Sewing schrieb:

    - wann verwende ich allgemein size_t und wann beim iterieren über einen container bspw. using sizet = std::vector::size_type? Wo besteht da der Unterschied?

    Wenn du immer size_t verwendest, dann wirst du selten ein Problem bekommen.
    Theoretisch kann container::size_type aber grösser sein als size_t. Beispielsweise wenn die C++ Implementierung es nicht unterstützt zusammenhängende Speicherblöcke zu verwenden die grösser als z.B. 2^32 Byte sind, der Adressraum aber 2^64 Byte hat, und daher Container wie list oder map durchaus mehr als 2^32 Elemente haben können.
    Bzw. früher zu DOS Zeiten waren die beiden Grenzen 2^16 und 2^32.
    Das ist aber kaum mehr relevant. Im Zweifelsfall fragst du einen Kollegen auf welchen Architekturen euer Code laufen muss und welche Eigenheiten die verwendeten Compiler/Systeme/... haben.

    Sewing schrieb:

    - Ich verstehe nicht, weshalb in setData der Speicher zweimal freigegeben wird. Wenn die if als true evaluiert, dann ist bereits Speicher alloziert worden, und den muss ich doch freigeben, da hier der shared_pointer doch nicht destroyed wird, oder wie?

    Wenn du einem shared_ptr etwas übergibst, dann gehört es ab diesem Moment shared_ptr, und du kannst es nie wieder "herauslösen". Es wird von shared_ptr zerstört werden, sobald es keinen shared_ptr mehr gibt der darauf zeigt.

    Durch die auf die manuelle Freigabe folgende Zuweisung _dPtr = sPtr; änderst du das worauf _dPtr zeigt. D.h. er zeigt dann nicht mehr auf das alte Objekt. D.h. es zeigt dann kein shared_ptr mehr auf das alte Objekt. Und das heisst das genau zu diesem Zeitpunkt das alte Objekt - das du davor schon manuell freigegeben hast - von shared_ptr zerstört wird.



  • Hmm, also.

    - setInternalMem() macht nur das, was man meint, wenn es vorher noch nie aufgerufen worden ist. Wär doch eher was für einen Konstruktor, oder?
    - der Kosntruktor ist wirklich ziemlich unnötig. Was soll der machen, also wozu muss denn dieser String (wenn da überhaupt was geloggt werden soll) ein Argument sein, reicht das nicht auch als lokale Variable?
    - Ich seh da einen virtual Destruktor, aber keine virtual members. Kann gewollt sein, aber brauchst du hier echt noch Vererbung?
    - Wenn ich setDataSize() und dann setInternalMem() aufrufe, kann ich danach wieder setDataSize() aufrufen und als Seiteneffekt ist mein interner Speicher dann futsch. Das finde ich insgesamt eine ziemlich fragwürdige API.

    Überhaupt... worin liegt der Sinn des ganzen? Kannst du nicht einfach einen std::vector<uint8> nehmen? Ja.. ok.. uninitialisiert, dann halt std::unique_ptr< std::vector<uint8> >. Das kapiert sofort *jeder* Entwickler, was man von deinem Konstrukt leider nicht gerade behaupten kann (sorry).

    Müsste auch in Autos funktionieren 🙂



  • Es geht um das Speichern der Daten von Sensoren. Diese Basisklasse ist allen Sensortypen gemein und wird dann eben durch Vererbung noch weiter spezifiziert.

    dann macht deine Klasse doch jetzt schon eindeutig viel zu viel - spielst du gerade mit OOP herum oder hast du damit Erfahrung?

    Es gibt einen riesen Unterschied zwischen:
    Inhalt: was sind die Roh-Daten eines Sensors und wie werden die gehalten (kann spezifisch sein)
    Verarbeitung: wie werden diese Roh-Daten in ein abnehmendes System konvertiert - z.B. Rohdaten zu double/string/...

    wenn du jetzt noch eine virtuelle convert_to_string einbaust hast du fast schon die Eierlegende Wollmilchsau der Sensor-Abstraktion - nur eben schon viel zu tief in deiner dann immer fetter werdenden Basis-Klasse

    Erst kommt das technologische Verständnis - so kann man es machen und das läuft auch, und dann irgendwann der Design-Anteil: so läuft es auch ist aber einfacher zu erweitern (ohne zu verfetten) und ist nicht so Code=>Fehlerlastig



  • Erstmal danke für eure ausführlichen Antworten,

    wenn der shared_ptr sein pointee zerstört, wird dann denn auch deallocate aufgerufen automatisch? ich verstehe sowieso nicht, wieso mir allocate nen built-in pointer zurück gibt, statt beispielsweise einem iterator oO

    Wann sind denn alternativen für heap speicher allokierung wenn man um new uint8[SIZE] syntax herum kommen will?

    Ist das std::vector da wirklich ne alternative oder bringt das zu viel overhead mit? und wann sollte ich diesen vector auf dem stack und wann mit nem smart-pointer auf dem heap anlegen? gibt es da guide lines?

    Ich werde mal versuchen eure Anregungne umzusetzen und dann kommt später auch nen konkretes beispiel, wenn ich soweit bin ; )



  • Sewing schrieb:

    built-in pointer zurück gibt, statt beispielsweise einem iterator

    Ein pointer ist ein Iterator.


  • Mod

    wenn der shared_ptr sein pointee zerstört, wird dann denn auch deallocate aufgerufen automatisch?

    Nein, der Deleter wird ausgeführt. Der hat auch mit deinem Allokator nur gemein, dass sie beide die Standardversionen sind, und damit delete aufrufen. Eleganter wäre es gewesen, einen Deleter vorzugeben, der deallocate ausführt.

    und wann sollte ich diesen vector auf dem stack und wann mit nem smart-pointer auf dem heap anlegen?

    ..was? vector alloziert seinen Speicher immer dynamisch. Ich sehe also nicht, warum du etwas was effektiv ein paar Zeiger sind, auf dem Heap anlegen solltest.

    Wann sind denn alternativen für heap speicher allokierung wenn man um new uint8[SIZE] syntax herum kommen will?

    Was kümmert dich denn Syntax? Wenn du Generizität möchtest, solltest du erst einmal einen Templateparameter für den Allokator bereitstellen.



  • Ist das std::vector da wirklich ne alternative oder bringt das zu viel overhead mit?

    std::vector hält (per default-allokator) seine Daten immer auf dem Heap, std::array ist dein Stack-Freund - jeweils mit default-Allokator

    Wann sind denn alternativen für heap speicher allokierung wenn man um new uint8[SIZE] syntax herum kommen will?

    warum? Das ist der Standard-Weg in C++, und wenn dein allokator wenigstens austauschbar wäre (Templare-Parameter) würde ich das wenigstens noch verstehen - aber einfach so einen std::allocator im Code ist irgendwie sinnfrei

    aus deinen Antworten lässt sich erkennen das dir relativ viel Grundlagen fehlen - auf der Basis so eine Klasse zu fabrizieren kann nur schlecht sein, lerne doch erstmal die Basics von C++/STL kennen bevor du von "Overhead" und "Um Syntax herum kommen" sprichst - ein erfahrener C++ Entwickler würde dein bisheriges Ergebnis so nicht nutzen wollen



  • hard, bard but fair.

    ich weiß, das vector auf dem heap alloziert, aber wann macht es sinn den vector durch nen smart pointer anzulegen und wann "normal" auf dem stack?



  • Sewing schrieb:

    wann macht es sinn den vector durch nen smart pointer anzulegen ?

    Ich lehne mich mal weit aus dem Fenster:
    Nie!



  • Sewing schrieb:

    wann macht es sinn den vector durch nen smart pointer anzulegen

    Wenn du sehr viele Objekte hast und die Existenz des vectors in deinem Objekt optional ist. Auf gcc/x64 ist sizeof(vector<int>)=24, ein Pointer aber hat nur size 8. Du kannst also 16 Bytes sparen, wenn du den vector dann nicht benutzt. Dafür hast du dann Overhead (sowohl Zeit, weil du einem Pointer mehr folgen musst) als auch Größe (nämlich der Extra-Pointer), falls du ihn benutzt.

    Außerdem kann es sinnvoll sein, wenn du auf die Daten im vector nur selten zugreifst, aber eine große Anzahl deiner Objekte in einer zeitkritischen Routine (die nicht auf den vector zugreift) im Cache sein sollen.


  • Mod

    wob schrieb:

    Sewing schrieb:

    wann macht es sinn den vector durch nen smart pointer anzulegen

    Wenn du sehr viele Objekte hast und die Existenz des vectors in deinem Objekt optional ist. Auf gcc/x64 ist sizeof(vector<int>)=24, ein Pointer aber hat nur size 8. Du kannst also 16 Bytes sparen, wenn du den vector dann nicht benutzt.

    Das ist ein Trugschluss. Das dynamische allozieren eines vector s birgt auch einen overhead in der Speicherverwaltung des Betriebssystems (was signifikant werden kann, sobald wir eben viele Objekte so anlegen). Der erwartete tatsächliche Speicherbedarf muss also nicht bei 20 liegen, sondern kann auch 24 übersteigen.


Log in to reply