18.03.21
Nach meinem dreiträgigen Krankenhausaufenthalt kann ich sagen: Gott, ich hasse es. Aber eine gute Sache hatte es: Ich hatte mehr als genug Zeit mir weitere Meldungen anzusehen. Ich dachte, ich zeige mal ein paar davon.
Zunächst hab ich mir PVS - Studio auf absolut legalem Wege gekauft um es nun für Metin2 nutzen zu können (hust hust). Nachdem ichs also gecrackt.. eh eh gekauft hatte, konnte ich mich mal etwas austoben. Dabei habe ich mich bis jetzt vor allem auf die Server Source & nur die Punkte mit der Priorität 'High' konzentriert.
Top 5: Unnötige Prüfungen
Im Sourecode wurden folgende Zeilen als 'unecessary' betitelt. Macht ja auch Sinn, der Wert von iNewRang wird niemals kleiner als 150 werden.
Über den Kommentar auf der rechten Seite wird ersichtlich, dass der Entwickler hier vergessen hat, die Prüfung rauszunehmen und vorher noch Berechnungen für Monster über deren Attack Range gemacht hatte.
Top 4: Nutzen von Pointern, die womöglich NULL sind (Führen unabdinglich zu Corecrashes)
Muss ich dazu noch was sagen? Falls kein Dungeon mit dieser ID gefunden wird, aus welchem Grund auch immer, derefernziere ich mit dem Pfeiloperator -> einen nullptr (NULL ist in clang 11 typedefed zu nullptr). Ich mache also womöglich:
zum Fixxen einfach die Zeile ein paar Zeilen runterschieben:
Davon gab es extrem viele, wobei man drauf achten muss was man genau verändert. Das Tool ist auch nicht fehlerlos, es erkennt manchmal z.B nicht das Pointer vor dem Aufruf von Funktionen bereits geprüft werden. So wie z.B hier, wo das Tool false positives ausgepuckt hat:
Das Tool hat gemeckert, dass pAttacker nullptr sein könnte. Kann es aber nicht, weil pAttacker beim Aufruf von CHARACTER::SendDamagePacket vorher geprüft wird auf Gülltigkeit. Was lernen wir daraus? Auch Tools sind nicht perfekt, immer schön eigene Gedanken machen. Nicht alles was automatisch erkannt wird, ist auch automatisch ein Fehlerfall in eurem Projekt.
Top 3: Memory Leaks
Der Klassiker, ich glaub das die YMIR Entwickler überhaupt nichts von Memory Leaks oder dieser Problematik wussten, anders kann ich mir diese Fehler einfach nicht erklären. In irgendeiner Funktion wird returned, ohne vorher das erstellte Objekt wieder zu löschen. Hier nur eins von 380.000 Beispielen in der Source:
Top 2: Unnötiges neu erstellen von Strings
Erklärt sich eigentlich auch selbst:
Ich habe einen std::string stStateFlag, diesen gebe ich als const char Pointer (const char*) über den Befehl c_str() an die Funktion GetFlag weiter. Dieser erstellt (je nachdem was die Min String Limit ist) diesen String und speichert ihn im Heap (ja für kleine Strings muss das nicht sein, unterscheidet sich je nach OS). Was ist effektiv passiert? Ich habe std::string zu const char* zu neuem std::string gemacht, das ist unnötig. Folgend wäre die Zeile korrekt:
Wenn die Funktion jetzt noch eine konstante Referenz auf den String entgegennimmt 'const std::string&', bin ich glücklich. Sonst wird der String auch wieder kopiert (also das selbe Prozedere), was unnötig ist.
Top 1: str.find() wird als boolean verwendet
Was ist falsch an diesem Ausdruck? Schauen wir uns dazu wieder die Funktion an auf cppreference.com:
Bitte melden Sie sich an, um diesen Anhang zu sehen.
Wieder ganz viele Möglichkeiten, wie man etwas suchen kann. Interessant ist vor allem, dass es einen Unterschied macht ob man einen Character sucht also:
oder einen const char* (der zu std::string bzw. basic_string& implizit konvertiert wird):
Laut quick-bench.com ist der Unterschied aufjedenfall gegeben. Hier mal ein Vergleich bei Clang11 (LLVM++11) ohne Optimierung:
Bitte melden Sie sich an, um diesen Anhang zu sehen.
Gut aber das nur nebenbei, also wir merken uns: Wenn wir nur einen Char suchen, wollen wir auch nur den Char suchen. Es macht einen Unterschied ob man '' oder "" nutzt. Check? Check.
Interessant für uns ist der Absatz Return Value:
Das heißt, wenn ich einen String "hallo" habe und "c" in diesem String suche, erhalte ich std::string::npos zurück und folgend steht dieser in der STL:
Ergo ich bekomme -1 (als Zahl zurück), der je nach size_type einen anderen Datentypen bekommt bzw. statisch in diesen gecasted wird. Diese Variable ist zur Compilezeit bekannt und static (static constexpr). Alles klar, was passiert also, wenn 'c' nicht gefunden wird, wie sieht unsere Abfrage von oben dann aus?
-1 ist aber im Boolschen kein Fehlerwert und wird nicht als solcher wahrgenommen, ergo das Ergebnis hier ist:
Dieser Ausdruck gibt also immer true zurück, egal ob c gefunden wurde oder nicht. Wie wäre es nun richtig? Naja, das sagt uns die Dokumentation ja, man soll auf std::string::npos prüfen, es wird schließlich std::string::size_type zurückgegeben und nicht bool. Also viel geredet, wie sieht die richtige Prüfung nun aus?
Geht doch. Ich bin jetzt ungefähr bei der Hälfte meiner Erkenntnisse aus dem Krankenhaus. Zyniker würden sagen, ich sollte öfter krank werden. Naja.
Weiter hab ich meine Source noch auf C++20 geupgraded, war eig. ganz einfach wenn man schon auf c++17 ist. Hier und da noch ein bisschen Korrektur und voilá.
Ich hab weiter noch Funktionen entfernt, die ich nicht nutze. So wie beim .DE Petsystem die Funktion HasOption sowie die Funktionen Mount() und Unmount() für Pets. Weiter hab ich alle dev_log's und das System als Solches entfernt. Und zuletzt noch die Variable '_dummy' in der guild.h, die nirgends gebraucht aber überall mitgegeben wird.
Grob gefasst wars das eigentlich schon, klar hab ich noch mehr gemacht - Aber daran kann ich mich nicht mehr errinern. Ich denke, da nun die Punkte durch sind die als 'Optimization' und 'High' geranked waren, könnt ich mich nun an die 'Middle' Punkte ranmachen. Ich weiß auch schon welche zwei Themen ich das nächste Mal thematisiere, aber für heute sind das genug Kopfschmerzen für euch.
Bis morgen!