Klassenfrage :)
-
Doch, es müssen selbstverständlich diese Objekte instanziiert werden, nur eben direkt und nicht als Zeiger:
Display display(...); Apple apple(...);
Aber du hast noch so viele Design- und Codefehler, daß es wirklich besser wäre, dieses Projekt noch mal neu aufzusetzen.
Hier eine (kurze) Liste der Verbesserungen:- Projektaufbau nach dem 3-Schichten-Modell: UI, Logik und Datenzugriff (Kurzer Überblick, wenn auch mit Java-Code: Wie funktioniert die 3-Schichten-Architektur?), d.h. die Klassen so organisieren, daß sie zu genau einer der Schichten gehören.
- Vermeidung von direktem Zugriff auf tiefere Objekte (Daten), wie z.B.
Apple->ui->display->xres
, d.h. Beachtung des Gesetz von Demeter. Kennen sollte man auch die anderen Prinzipien des Artikels (zumindestens von SOLID sollte man schon gehört haben und diese bei OOP auch beachten). - Nur die Header je Datei einbinden, die auch direkt benötigt werden - und nicht, wie in "_Display.h", alle Systemheader einbinden! In Headerdateien reichen auch meistens Vorwärtsdeklarationen.
Insgesamt ist dein Code auch viel zu lang (für die eigentliche Funktionalität), d.h. es erscheint mir eher eine Copy&Paste-Programmierung zu sein, anstatt Aufteilung in passende (kleinere) Funktionen.
Und dann kommt hinzu, daß eine Umsetzung von C nach C++ nicht bloß das Austauschen von Funktionen bzw. Typen ist (dies wird oft als "C mit Klassen" bzw. "C mit cout" bezeichnet), sondern bei (modernem) C++ werden auch andere Techniken eingesetzt, z.B.
- RAII
- Smart-Pointer (der deutsche Artikel Intelligenter Zeiger ist nur sehr kurz).
- Template
- ...
-
@Schlangenmensch @Finnegan @Th69
Ich hab es ohne "new" compiliert bekommen, aber es läuft sofort in einen Speicherfehler.
-
@Th69
Copy and Paste ist mein Mittel der Tippfehlervermeidung!
Und Die Paradigmen Anderer zur Umsetzung eigener Programme ist sinnvoll wenn man die Paradigmen schon kennt, man sucht doch aber nicht nach solchen sondern wird kreativ.
Sicher sind Funktionen des Programms optimierbar, aber das sind (mir) im moment "Kinkerlitzchen"
-
@EL-europ sagte in Klassenfrage :
@Schlangenmensch @Finnegan @Th69
Ich hab es ohne "new" compiliert bekommen, aber es läuft sofort in einen Speicherfehler.Die Ursache dafür wirst du schon selbst mit den genannten Werkzeugen herausfinden und beheben müssen, da dein Programm nicht nur ein Fraktal generieren möchte, sondern obendrein scheinbar auch noch fraktale Fehler erzeugt ( ), die es sehr schwer machen für jemanden, der nicht genau dasselbe Setup fährt, diesen Fehler nachzuvollziehen (siehe unten, mein Test unter WSL2). Eine IDE mit einem integrierten Debugger ist da übrigens ganz hilfreich - Puristen können
gdb
auch über die Kommandozeile verwenden (davon weiss ich aber nicht allzu viel).Wenn du das tatsächlich mit deinem aktuellen Code weiter durchziehen willst, solltest du Sanitizer und Warnungen aktivieren (inklusive
-Werror
), die auftretenden Fehler einen nach dem anderen ausfindig machen, herausfinden weshalb sie auftreten und die Ursache beseitigen - und dabei natürlich überlegen ob und wie man es vielleicht besser machen könnte. Anders sehe ich nicht, wie du mit dem Programm auf einen grünen Zweig kommen kannst.Einen Speicherfehler bekomme ich beim Testen übrigens auch, aber sehr wahrscheinlich aus völlig anderen Gründen wie bei dir. Bei mir unter Windows mit WSL2 existiert z.B. das Framebuffer-Device (
/dev/fb0
) nicht, daher gibtopen("/dev/fb0", O_RDWR)
inDisplay.cpp
bei mir-1
zurück undstrerror(errno)
den StringNo such file or directory
.Leider pfüfst du danach nur, ob
fbfile
ungleich0
ist, was du als "kein Fehler" interpretierst (siehe Doku zuopen
, meiner Ansicht nach ist nicht nur alles was kleiner als0
ebenfalls ein Fehler, sondern auch noch0
ein gültiger File-Deskriptor, also kein Fehler) und dein Code macht munter weiter als wäre nichts gewesenAuch mit Fehlerbehandlungen wie in solchen Zeilen:
if (ioctl(fbfile, FBIOGET_FSCREENINFO, &Finfo)){return;} if (ioctl(fbfile, FBIOGET_VSCREENINFO, &Vinfo)){return;}
Damit machst du dir und anderen nur das Leben schwer, da hat man praktisch keine Chance als Nutzer der Display-Klasse herauszufinden, ob die Initialisierung nun erfolgreich war oder nicht - geschweige denn warum sie überhaupt fehlgeschlagen ist.
Eine Möglichkeit, solche Fehler sauber zu handhaben wären Exceptions. Das könnte dann z.B. so oder ähnlich aussehen:
_Display.cpp
... #include <string> #include <cstring> #include <stdexcept> ... Display::Display() { ... fbfile = open("/dev/fb0", O_RDWR); if (fbfile < 0) throw std::runtime_error{ std::string{ "Unable to open '/dev/fb0': " } + std::strerror(errno) }; ... if ( ioctl(fbfile, FBIOGET_FSCREENINFO, &Finfo) < 0 || ioctl(fbfile, FBIOGET_VSCREENINFO, &Vinfo) < 0 ) throw std::runtime_error{ std::string{ "Failed to acquire framebuffer screen information for '/dev/fb0': " } + std::strerror(errno) }; ...
mandel.cpp
:... #include <iostream> #include <stdexcept> ... int main() { try { ... //Programmende return 0; } catch (const std::exception& e) { std::cerr << "ERROR: " << e.what() << std::endl; return 1; } catch (...) { std::cerr << "Unexpected exception!" << std::endl; return 2; } }
Edit: Exceptions sind natürlich noch ein Grund mehr auf manuelle Speicherverwaltung zu verzichten. Stichwort Exception Saftety. Bezogen auf die
Display
-Klasse könnte manfbspiegel
wahrscheinlich alsstd::vector<uint8_t>
umsetzen und das Memory Mapping (mmap/munmap
) über einenstd::unique_ptr<uint8_t>
mit Custom Deleter erschlagen.Z.B. so oder ähnlich (ungetestet):
#include <memory> class Display { ... std::unique_ptr<std::uint8_t> fbpointer; ... }; Display::Display() { ... fbpointer = std::unique_ptr{ static_cast<std::uint8_t*>( mmap( 0, bufferlength, PROT_READ | PROT_WRITE, MAP_SHARED, fbfile, 0 ) ), [&](std::uint8_t* ptr){ { munmap(ptr, bufferlength); } }; ... }
Der
std::unique_ptr
ruft nämlich automatisch den Deleter (hier die Lambda-Funktion) auf, wenn er zerstört wird. Auch dann, wennDisplay
nicht vollständig konstruiert wurde.Das Problem, wenn im Konstruktor eine Exception fliegt ist nämlich, dass zwar für alle bereits konstruierten Member-Variablen die Destruktoren aufgerufen werden, nicht aber für
Display
selbst. Das macht es schwierig, mit manuellemmalloc
/free
odermap/unmap
den Konstrukor exception-sicher hinzubekommen, so dass die bis dahin reservierten Ressourcen auch alle wieder freigegeben werden.Die Lösung ist hier RAII zu verwenden, z.B. mit dem genannten
std::vector
und demstd::unique_ptr
. Dann klappts auch, wenn die Ausnahmen fliegen
-
@Finnegan
Großen Dank für deine Hinweise, vor allem try/catch is teine gute Idee (den Rest muss ich noch genauer betrachten). Erfahrene Programmierer berücksichtigen schon zu Beginn die "Allgemeingültigkeit" ihres Codes, ich bin tatsächlich nicht daran interressiert gewesen (will aber kein unerfahrener bleiben). Das ich das sanitizer Flag nicht bei den Objektdateien setzen, sondern oben im makefile (beim Linker laube ich) hab ich grad rausgefunden.
Ich war Elektriker der mit windos in Pascal und VB programmiert hatte, bis ich merkte das ich keinen Zugriff auf die Hardware bekomme und auf Linux umstieg; das dauerte Jahre weil kein Anderer (in meinem Umfald) Linux nutzte. Und ich machte direkt den Fehler mit Java (um die Jahrtausendwende) zu beginnen, bis ich merkte von Fremdbibliotheken abzuhängen und von Hardware noch immer sehr weit entfernt bin (es waren ja alles nur Blackboxen für mich). Aber die Bibliotheken waren von Java und MS Büchern geflutet und nix über c (in Deutsch). Ich bin also ein Irrender der sich in die Visualisierung der Mandelbrodtmenge verliebt hat und Filme, mit geilen Beats untermalt, davon machen will. Aber ich bin auch ein Schwamm der aus eigenen und Fehlern Anderer lernt.
Deine Hinweise zu den std::vector Methoden hab ich umgesetzt bekommen, aber wenn ich ohne "=new" das prog compiliert bekomme läuft es direkt in einen Speicherfehler. Mit dem sanitizer Falg muss ich jetzt erst ma spielen um es nutzen zu können, deshalb werden erst alle Anderen brauchbaren Hinweise umgesetzt. Dank Allen hier, auch den Trollen
-
Ja. Stelle jedoch sicher, die Prinzipien der OOP nicht zu verletzen. Um dem gerecht zu werden gäbe es einmal die Möglichkeit der Vererbung und auch Mehrfachvererbung was einen gewissen Verwandtschaftsgrad der Klassen voraussetzt. Lässt sich ein verwandschaftlicher Bezug der Klassen nicht herstellen, ist es möglich, die Methoden fremder Klassen zu delegieren, also quasi zu eigenen Methoden zu machen. Hierzu bekommt die eigene Instanz als Eigenschaft eine Instanz der fremden Klasse (s.u). Das ermöglicht sogar, der eigenen Methode den gleichen Namen derjenigen fremden Methode zu geben die dann aufgerufen wird. Man kann das auch so machen, daß die Instanz der fremden Klasse erst erstellt wird, wenn die Methode der eigenen Klasse aufgerufen wird (lazy delegation).
Anm.: Aggregation
-
@Finnegan
fbpointer als std::unique_ptr zu deklarieren funz bei mir nicht, auch wenn ich eine "{"-Klammer hinzufüge oder wegnehme bringt er mir "include/_Display.cpp:43:17: error: class template argument deduction failed:" Das schwerwiegende Problem sind die "Globalen" thr-Variablen in der _Apple Klasse. Denn erst beim Aufbau des Objekts Apple beginnt sanitaze::address abzubrechen, das Ui wird dargestellt der Rest nicht.
-
@_ro_ro
Dein Post gibt mir schon zu Denken, doch das einzige was dem Programm (abgesehen der Abbrüchen) noch fehlt ist Funktionalität, und die bekomme ich mit dem Desing hin wenn ich doch nur die Thread Funktion in c++ sauber hinbekäme.
Die unterteilt die x-Achse in gleichgroße Abschnitte und schickt dem Abschnitt entsprechend viele Threads los um die y-Werte mit einer "getIterFuntion" zu berechnen. Ich habs mit dem Wolf und dem Netz versucht, doch die Beschreibungen waren(sind) mir sehr Abstrakt. Jetzt werd ich erst mal zumindes den Rest mit den exceptions umsetzten und versuch heraus zu finden ob das erste catch ausreicht
-
@EL-europ sagte in Klassenfrage :
@Finnegan
Großen Dank für deine Hinweise, vor allem try/catch is teine gute Idee (den Rest muss ich noch genauer betrachten). Erfahrene Programmierer berücksichtigen schon zu Beginn die "Allgemeingültigkeit" ihres Codes, ich bin tatsächlich nicht daran interressiert gewesen (will aber kein unerfahrener bleiben).Ich verstehe den Gedanken dahinter, aber letztendlich machst du es dir auch selbst einfacher, wenn du diese Sachen alle berücksichtigst, weil du dich dann deutlich weniger mit solchen Problemen herumärgern musst. Selbst wenn es nur ein persönliches Projekt ist
Das ich das sanitizer Flag nicht bei den Objektdateien setzen, sondern oben im makefile (beim Linker laube ich) hab ich grad rausgefunden.
Du könntest in deinem Makefile auch mit Umgebungsvariablen arbeiten. Sowas hier ist z.B. verbreitet:
mandelbrodt: mandel.o _Display.o _Userinterface.o _Apple.o _Colorinterface.o $(CXX) $(CXXFLAGS) -o mandelbrodt mandel.o _Display.o _Userinterface.o _Apple.o _Colorinterface.o make clean mandel.o: mandel.cpp $(CXX) $(CXXFLAGS) -lpthread -c mandel.cpp _Display.o: include/_Display.cpp $(CXX) $(CXXFLAGS) -c include/_Display.cpp _Userinterface.o: include/_Userinterface.cpp $(CXX) $(CXXFLAGS) -c include/_Userinterface.cpp _Apple.o: include/_Apple.cpp $(CXX) $(CXXFLAGS) -c include/_Apple.cpp _Colorinterface.o: include/_Colorinterface.cpp $(CXX) $(CXXFLAGS) -c include/_Colorinterface.cpp clean: rm -f mandel.o rm -f _Display.o rm -f _Userinterface.o rm -f _Apple.o rm -f _Colorinterface.o
Dann kannst du sehr einfach in der Kommandozeile die Flags oder auch den Compiler ändern.
CXX
ist meines wissens sogar per Default definiert, d.h. das sollte auch direkt funktionieren, ohne irgendwelche Variablen zu setzen:> export CXXFLAGS="-std=c++20 -g -fsanitize=address -fsanitize=leak -fsanitize=undefined -Wall -Wpedantic" > export CXX=clang++ > make
Ich war Elektriker der mit windos in Pascal und VB programmiert hatte, [...]
Hah, mit Pascal hab ich seinerzeit auch angefangen, dann aber schnell mit Assembler weitergemacht, u.a. weil (Turbo) Pascal nen schönen Inline-Assembler hatte. Danach kamen dann, C, C++, Java und Jede Menge andere Sprachen. Am Anfang hab ich auch ne Menge Schrott programmiert und viel rumexperimentiert (als Schüler), daher kann ich mich damit durchaus identifizieren ... trotzdem, je schneller man sich bewährtes Handwerkszeug wie hier im Thread besprochen angewöhnt, umso besser.
Deine Hinweise zu den std::vector Methoden hab ich umgesetzt bekommen, aber wenn ich ohne "=new" das prog compiliert bekomme läuft es direkt in einen Speicherfehler. Mit dem sanitizer Falg muss ich jetzt erst ma spielen um es nutzen zu können, deshalb werden erst alle Anderen brauchbaren Hinweise umgesetzt. Dank Allen hier, auch den Trollen
Wie gesagt, aktivier mal alle Flags die ich vorgeschalgen habe und versuche alle Fehler und Warnungen zu verstehen und zu beseitigen. ich denke dabei lernt man eine Menge. Danach macht es wahrscheinlich Sinn das alles nochmal mit dem neuen Wissen neu zu schreiben - ein gutes Buch hilft natürlich auch
-
@Finnegan
Du hast wohl recht, doch will ich noch am dem Code kämpfen. Ein neues Buch wird wohl auch fällig, das muss ich mir erst vom Munde absparen
-
@EL-europ sagte in Klassenfrage :
@Finnegan
fbpointer als std::unique_ptr zu deklarieren funz bei mir nicht, auch wenn ich eine "{"-Klammer hinzufüge oder wegnehme bringt er mir "include/_Display.cpp:43:17: error: class template argument deduction failed:" Das schwerwiegende Problem sind die "Globalen" thr-Variablen in der _Apple Klasse. Denn erst beim Aufbau des Objekts Apple beginnt sanitaze::address abzubrechen, das Ui wird dargestellt der Rest nicht.Ja, die Sache mit dem
fbpointer
habe ich gerade mal ausprobiert und bin da auch auf ein paar Probleme gestossen. Zum einen habe ich die Template-Argumente nicht richtig angegeben und im Display-Konstruktor auch vergessen, so dass er dort versucht die zu deduzieren.Eigentlich müsste der Typ ein
std::unique_ptr<std::uint8_t, void(*)(std::uint8_t*)>
sein, also ein (unique) Pointer aufstd::uint8_t
mit einem Funktionszeiger auf einevoid (std::uint8_t*)
-Funktion als "Deleter".Bei dem Lambda ist mir dann aufgefallen, dass wenn ein Capture benötigt wird (für
bufferlength
) die Lambda-Funktion nicht mehr kompatibel mit diesem Funktionspointer ist.Man kann auch ein Funktionsobjekt (Funktor) schreiben für den Deleter, diese Klasse kann man dann als "Deleter-Typ" für
unique_ptr
angeben. Das ist dann aber nicht mehr so schön kompakt.Leider ist meine Zeit heute und in den nächsten paar Tagen sehr knapp bemessen, ich weiss nicht ob ich mir nochmal genau anschauen kann, wie man das am besten löst.
Vielleicht hat ja jemand anderes hier eine Idee (?)
-
@Finnegan
Dank für deine Müh.
-
Ich hab das c-array fbspiegel in ein std::vector geändert und alle Aufrufe natürlich angepasst; die Geschwindigkeit ist untragbar langsam gegenüber dem c-array, der Bildaufbau ist betrachtbar.
-
@EL-europ sagte in Klassenfrage :
Ich hab das c-array fbspiegel in ein std::vector geändert und alle Aufrufe natürlich angepasst; die Geschwindigkeit ist untragbar langsam gegenüber dem c-array, der Bildaufbau ist betrachtbar.
Es gibt keinen technischen Grund, weshalb ein
std::vector
langsamer als ein dynamisches C-Array sein sollte, zumal man im Zweifelsfall mitvector.data()
auch direkten Pointerzugriff auf den Speicherbereich bekommen kann.Eventuell verwendest du den
vector
auf eine ineffiziente Weise.vector.at()
macht z.B. noch einen Range Check und wirft eine Exception bei einem Zugriff außerhalb des Bereichs. Wenn man damit Grafik mit vielen Millionen Pixeln machen will (und vielleicht sogar noch Animationen), dann kann das IMHO schon zu einer sichtbaren Verlangsamung führen.Auch die Sanitizer (falls aktiviert) lassen den Code bestimmt nicht schneller werden.
Mach nochmal ein Commit auf Github und wenn ich die Tage wieder Zeit finde, kann ich mir das mal ansehen.
-
@Finnegan
Ja ich hab fpspiegel im Konstruktor mit "fbspiegel.resize(bufferlenght)" initialisiert und in den Methoden mit fbspiegel.at() darauf zugegriffen. Nach den Ergebnissen hab ich es aber wieder rückgängig gemacht, und erst mal nach deinem Hinweis dem Apple ein AppleParas als Parameter gegen die Einzelwerte getauscht, so das die mandel.cpp etwas aufgeräumter aussieht.
mit den sanitizern läuft es mir immer in einen Fehler. Auch sanitize::leak zeigt das Apple mit ui->textFertig() Probleme macht; auch nachdem ich die sprintf()-Anweisung dort durch "Festwerte" ersetzt hatte.
Liegt es vielleicht daran das ich kein "this->" in den Methoden verwende?
-
@EL-europ sagte in Klassenfrage :
@Finnegan
Ja ich hab fpspiegel im Konstruktor mit "fbspiegel.resize(bufferlenght)" initialisiert und in den Methoden mit fbspiegel.at() darauf zugegriffen.Das ist prinzipiell ja auch gut und richtig, besonders wenn du so viele Probleme mit Pufferüberläufen hast. Aber es geht halt ein wenig auf die Performance. Das ist für viele Anwendungen nicht relevant, aber bei einem Grafik-Buffer würde ich da vielleicht eine Ausnahme machen.
Dennoch sollte man bei Indexberechnungen sorgfältig darauf achten, dass die Buffer-Grenzen nicht über- oder unterschritten werden. Ich würde da wohl viel mit
assert
arbeiten (diese Prüfungen sind nur im Debug-Modus aktiv, bzw. wennNDEBUG
nicht definiert ist) und ansonsten z.B.fpspiegel[index]
verwenden.mit den sanitizern läuft es mir immer in einen Fehler. Auch sanitize::leak zeigt das Apple mit ui->textFertig() Probleme macht; auch nachdem ich die sprintf()-Anweisung dort durch "Festwerte" ersetzt hatte.
Du solltest versuchen, die Meldungen des Sanitizers zu verstehen und das Problem zu beseitigen. Das sind alles Bugs mit mehr oder minder schwerwiegenden Auswirkungen. Wenn das trotzdem "irgendwie" läuft, dann ist das nur Glück
Wenn du Probleme mit englissprachigen Meldungen haben solltest, dann versuchs mal mit ner Maschinenübersetzung (DeepL oder sowas) oder frag hier im Forum nochmal nach. Die Sanitizer-Fehler solltest du schon alle beheben, das erachte ich als Minimum. bei Compiler-Warnungen würd ich vielleicht die eine oder andere durchgehen lassen (z.B. harmlose wie dass man eine Varibale deklariert hat, die nirgendwo verwendet wird).
Liegt es vielleicht daran das ich kein "this->" in den Methoden verwende?
Ich denke nicht, solange du dadurch nicht versehentlich auf eine andere Variable zugreifst, welche eine Member-Variable überdeckt. Z.B. ein Funktionsparameter oder eine lokale Variable mit dem selben Namen. Ich verwende auch meist nur den Namen der Member-variablen und
this->variable
(oder alternativKlasse::variable
) wenn es nicht eindeutig ist.
-
@Finnegan
OK, ich setzt mich mit den sanitizer-Meldungen auseinander, aber das ist ein Studuim für mich und dauert ein wenig. Hab schon, gegen meine Behauptung, wieder an der Colorinterface gearbeitet
-
@EL-europ sagte in Klassenfrage :
@Finnegan
OK, ich setzt mich mit den sanitizer-Meldungen auseinander, aber das ist ein Studuim für mich und dauert ein wenig. Hab schon, gegen meine Behauptung, wieder an der Colorinterface gearbeitetVielleicht ist es leichter, erstmal die Compiler-Warnungen zu beseitigen. Gut möglich, dass sich dann einige Sanitizer-Fehler in Wohlgefallen auflösen
-
Das sieht man doch schon am Code allein, daß dort noch viele Speicherlecks sind (da zu den ganzen
malloc
- bzw.realloc
-Aufrufen diefree
-Aufrufe fehlen).Und was versprichst du dir von dem
NULL
setzen?// in Apple::calc() thr_matrix = NULL; thr_colors = NULL; thr_matrix = (int**)malloc(xres * sizeof(int*)); thr_colors = (int**)malloc(xres * sizeof(int*));
Ich denke auch, daß sich der Sanitizer darauf bezieht und nicht auf die
ui->textFertig(false);
-Zeile vorher.Bei Ersetzen durch einen
vector<...>
fallen diese Probleme dann aber weg.PS: Wenn du viele Zugriffe auf ein
vector<...>
-Element hast, dann erzeuge eine Referenz-Variable, anstatt x-mal auf dasselbe Element zuzugreifen, wie z.B. in "_Colorinterface::drawElem(...)" mitelements.at(apos)
:auto& element = elements.at(apos); // bzw. elements[apos]
(falls du noch nicht C++11 oder höher benutzt, mußt du den konkreten Typen anstatt auto angeben).
-
@Th69
Den Speicher gebe ich im Destruktor von Apple wieder frei, in der Hoffnung das es so funktioniert; denke aber auch das in den "rohen arrays" das Problem liegt. Ich werd jetzt nochmals den fbpiegel in Userinterface in ein std::vector ändern und ohne at() darauf zugreifen. Die performance muss gut sein sonst wird das prog unbrauchbar. Das bedeuted: wenn die Performance nicht gut ist muss das Prog um die Rohen Arrays "herum" geschrieben werden weil sie bleiben müssen (so meine Intuition)