Codeteile auslagern. Bekomme cpplint Warnung
-
@MegaV0lt
Die beiden Funktionen sind absolutes No-Go, du solltest dich unbedingt in das Thema Smart Pointer einlesen.#include <memory> // shared_ptr ist der bequemere Smart Pointer, vllt ist std::unique_ptr die bessere Alternative std::shared_ptr<cSkinDisplayChannel> cFlat::DisplayChannel(bool WithInfo) { return std::make_shared<cFlatDisplayChannel>( WithInfo ); } std::shared_ptr<cOsdItem> cMenuSetupSubMenu::InfoItem(const char *label, const char *value) { // was ist wenn, min. einer der beiden Parameter nullptr ist? std::shared_ptr<cOsdItem> retval = std::make_shared<cOsdItem>( cString::sprintf("%s: %s", label, value) ); retval->SetSelectable(false); return retval; }
Ein paar andere Dinge:
- Die Ungarische Notation gilt als veraltet und sollte nicht mehr benutzt werden. Also besser
OsdItem
undFlatDisplayChannel
stattcOsdItem
undcFlatDisplayChannel
. - Warum benutzt du
const char*
für Zeichenketten statt std::string? - Für fast alle Arrayallokationen kann
std::vector
benutzt werden. Vorsicht beistd::vector<bool>
, der nimmt eine Sonderstellung ein, weil er sich anders verhält.
#include <vector> // statt char *p = (char *) malloc(hor_filters_size + ver_filters_size + buffer_size); // besser std::vector, den vector als Parameter übergeben, falls notwendig std::vector<char> buffer( hor_filters_size + ver_filters_size + buffer_size); // Zugriff auf den Puffer char* p = buffer.data();
Wenn du iwo
new
odermalloc
siehst solltest du dir überlegen, warum das gemacht wird und wie das C++ Pendant dazu aussieht. Oder ob man überhaupt dynamischen Speicher braucht und Objekte nicht auf dem Stack erzeugen kann:Epgsearch_lastconflictinfo_v1_0 *serviceData = new Epgsearch_lastconflictinfo_v1_0; if( service_data ) { } delete serviceData; // ist in mehrerer Hinsicht falsch: // 1) new gibt im Fehlerfall keinen nullptr zurück, sondern wirft eine bad_alloc exception. Die Überprüfung des Rückgabewertes ist also sinnlos // 2) warum wird service_data überhaupt auf dem Heap erzeugt und nicht auf dem Stack? Nur weil iwo ein Zeiger als Parameter erwartet wird heißt // das nicht, dass das Objekt auf dem Heap erzeugt werden muss. Epgsearch_lastconflictinfo_v1_0 serviceData; serviceData.nextConflict = 0; serviceData.relevantConflicts = 0; serviceData.totalConflicts = 0; p->Service("Epgsearch-lastconflictinfo-v1.0", &serviceData); if (serviceData.relevantConflicts > 0) { numConflicts = serviceData.relevantConflicts; }
Irgendwie sieht das alles nach C und nicht C++ aus.
Edit:
Das Konzept const-correctness solltest du dir auch unbedingt anschauen.Edit 2:
service_data => serviceData
- Die Ungarische Notation gilt als veraltet und sollte nicht mehr benutzt werden. Also besser
-
@DocShoe sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Irgendwie sieht das alles nach C und nicht C++ aus.
Die Datei heißt laut @MegaV0lt ja auch
imagescaler.c
... Weist also sehr sehr stark darauf hin, dass der Code in C geschrieben ist und nicht in C++.
-
@wob sagte in Codeteile auslagern. Bekomme cpplint Warnung:
@DocShoe sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Irgendwie sieht das alles nach C und nicht C++ aus.
Die Datei heißt laut @MegaV0lt ja auch
imagescaler.c
... Weist also sehr sehr stark darauf hin, dass der Code in C geschrieben ist und nicht in C++.Nur das in dem file auch c++ code sich befindet (Verwendung von new oder von Klassen z.b. cIndexFile ).
Aktuell sieht das ganze nach ein Gemisch von C und C++ code aus.
-
@wob sagte in Codeteile auslagern. Bekomme cpplint Warnung:
@DocShoe sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Irgendwie sieht das alles nach C und nicht C++ aus.
Die Datei heißt laut @MegaV0lt ja auch
imagescaler.c
... Weist also sehr sehr stark darauf hin, dass der Code in C geschrieben ist und nicht in C++.Und wie passt das mit
new
/delete
zusammen? Also entweder C (und dann bitte auch in's passende Unterforum verschieben), oder C++ (dann bitte Dateien entsprechend benennen).@MegaV0lt
Hab mir das Projekt im Detail nicht angeguckt, aber was machst du da eigentlich? Baust du irgendwas um bestehenden C-Code oder kommt der komplette Quelltext von dir?
-
Vielen Dank für die Antworten. Das ist ein altes Projekt, das ohne Entwickler zurück blieb und ich es notgedrungen übernommen habe. Leider bin ich kein Programmierer und hangle mich so durch. Der Code wird aber mit gcc übersetzt als 'c11' Code übersetzt.
Habe schon ein paar Änderungen oder Erweiterungen eingebaut. Ich werde aktiv, wenn am VDR oder den Plugins, welche das Skin auch unterstützt geändert werden, oder ich eine Option einbauen will wie zum Beispiel die Anzeige der Fehler in der Aufnahme mit oder ohne Symbol. Versuche auch den Code zu verstehen oder im Netz nach Infos oder Hilfe zu suchen.
Werde versuchen die Hinweise und Tipps umzusetzen.Warum das alles mal so gemacht wurde, kann ich nicht sagen.
Ich denke schon, dass es in den c++ Bereich soll, da es eine Mischung aus C und C++ ist
-
@DocShoe sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Die beiden Funktionen sind absolutes No-Go, du solltest dich unbedingt in das Thema Smart Pointer einlesen.
Leider bekomme ich das nicht hin, schade.
g++ -Wall -g -O3 -Wall -Werror=overloaded-virtual -Wno-parentheses -fPIC -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -std=c++11 -c -DPLUGIN_NAME_I18N='"skinflatplus"' -DVDRLOGO=\"vdrlogo_yavdr\" -DWIDGETFOLDER='"/usr/lib/vdr/plugins/skinflatplus/widgets"' -I/usr/include/uuid -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/GraphicsMagick -o skinflatplus.o skinflatplus.c flat.c:57:38: error: no declaration matches ‘std::shared_ptr<cSkinDisplayChannel> cFlat::DisplayChannel(bool)’ 57 | std::shared_ptr<cSkinDisplayChannel> cFlat::DisplayChannel(bool WithInfo) { | ^~~~~ In file included from flat.c:5: ./flat.h:202:38: note: candidate is: ‘virtual cSkinDisplayChannel* cFlat::DisplayChannel(bool)’ 202 | virtual cSkinDisplayChannel *DisplayChannel(bool WithInfo); | ^~~~~~~~~~~~~~ ./flat.h:196:7: note: ‘class cFlat’ defined here 196 | class cFlat : public cSkin { | ^~~~~ flat.c:61:27: error: ‘cMenuSetupSubMenu’ has not been declared 61 | std::shared_ptr<cOsdItem> cMenuSetupSubMenu::InfoItem(const char *label, const char *value) { | ^~~~~~~~~~~~~~~~~ flat.c: In function ‘std::shared_ptr<cOsdItem> InfoItem(const char*, const char*)’: flat.c:66:5: error: ‘displayMenu’ was not declared in this scope; did you mean ‘cSkinDisplayMenu’? 66 | displayMenu = retval; | ^~~~~~~~~~~ | cSkinDisplayMenu make[1]: *** [Makefile:102: flat.o] Fehler 1
scheint complexer zu sein.
Aus der flat.h:
class cFlat : public cSkin { private: cFlatDisplayMenu *displayMenu; public: cFlat(void); virtual const char *Description(void); virtual cSkinDisplayChannel *DisplayChannel(bool WithInfo); virtual cSkinDisplayMenu *DisplayMenu(void); virtual cSkinDisplayReplay *DisplayReplay(bool ModeOnly); virtual cSkinDisplayVolume *DisplayVolume(void); virtual cSkinDisplayTracks *DisplayTracks(const char *Title, int NumTracks, const char * const *Tracks); virtual cSkinDisplayMessage *DisplayMessage(void); };
-
Ok, ich habe mir die Sourcen des Hauptprojektes tvdr.de angeschaut. Da herrscht ebenfalls dieses Wirrwarr. Persönlich sehe ich da keine große Chance ein Plugin auf saubere C++ umzustellen, wenn das Hauptprojekt schon so wirr ist.
@MegaV0lt Mein Beileid, aber da hast Du Dir einen sehr hässlichen Brocken für ein Anfängerprojekt ans Land gezogen.
-
@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Der Code wird aber mit gcc übersetzt als 'c11' Code übersetzt.
Irgendwie erscheint mir das komisch. Wenn du das als C-Code übersetzt kann das mit dem
class
Keyword schon nicht funktionieren.
In C Code kannst du auch keine Smartpointer einsetzen.Deine Fehlermeldung kommt daher, dass du die Funktionsdefinition in der Source Datei geändert hast, aber nicht die Deklaration im Header.
Du kannst mit C Compilern keinen C++ Code kompilieren. C++ unterstützt einige C-Feature, aber da ist Vorsicht geboten, nicht alles, was man aus C kennt, bedeutet in C++ dasselbe. Du musst schon genau wissen, welche Sprache mit welchem Standard du verwenden willst
-
@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:
-std=c++11
Das sollte doch für c++ sein?
Ich habe das gemacht, weil ich das Skin-Plugin selber nutze und nicht möchte, dass es irgendwann obsolet wird.Zum Glück habe ich bis jetzt alles so weit mit Hilfen zum laufen gebracht und auch einige Sachen neu eingebaut und "optimiert". Beispiel Das Laden der Bilder für die Darsteller wurde immer zwei mal durchlaufen, um festzustellen, ob der Inhalt mit oder ohne Scrollbalken angezeigt wird. Hab es so umgeschrieben, dass angenommen wird, dass Scrollbalken benötigt werden und falls nicht die zweite Schleife auf zwischengespeicherte Werte zugreift...
Leider habe ich auch nicht all zu viel Zeit um mich tiefer einzuarbeiten, aber ich bin trotzdem einigermaßen zufrieden, dass es stabil läuft und macht was es soll.
Vielen Dank noch mal für die vielen Tipps und Vorschläge
-
@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Der Code wird aber mit gcc übersetzt als 'c11' Code übersetzt.
@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:
-std=c++11
c11 != c++11. Daher die Verwirrung bei uns ob du nun C++ oder C machst.
-
Ja, das zieht sich dann quer durch die Vererbungshierachie. Die Umstellung auf shared_ptr erfordert dann, dass die Signaturen der Basisklasse angepasst werden. Das kann schnell eskalieren
-
@DocShoe sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Epgsearch_lastconflictinfo_v1_0 *serviceData = new Epgsearch_lastconflictinfo_v1_0;
if( service_data )
{
}
delete service_data;// ist in mehrerer Hinsicht falsch:
// 1) new gibt im Fehlerfall keinen nullptr zurück, sondern wirft eine bad_alloc exception. Die Überprüfung des Rückgabewertes ist also sinnlos
// 2) warum wird service_data überhaupt auf dem Heap erzeugt und nicht auf dem Stack? Nur weil iwo ein Zeiger als Parameter erwartet wird heißt
// das nicht, dass das Objekt auf dem Heap erzeugt werden muss.
Epgsearch_lastconflictinfo_v1_0 serviceData;
serviceData.nextConflict = 0;
serviceData.relevantConflicts = 0;
serviceData.totalConflicts = 0;
p->Service("Epgsearch-lastconflictinfo-v1.0", &serviceData);
if (serviceData.relevantConflicts > 0)
{
numConflicts = serviceData.relevantConflicts;Das is auch so eine Altlast. Kann ich da das new einfach weglassen oder muss das dann ganz anders gemacht werden?
-
Der dort stehende Code ist doch schon die korrigierte Version (oder siehst du da noch ein
new
?).Es gäbe nur einen Grund, dort
serviceData
im Freispeicher (Heap) zu allozieren, nämlich wenn dieService
-Funktion den übergebenen Zeiger langfristig speichern würde (und dann außerhalb der aufrufenden Funktion noch mal darauf zugegriffen würde) - aber dann dürfte dort auch keindelete serviceData
(im Code von @DocShoe steht ja fälschlicherweiseservice_data
;- ) stehen.
-
@Th69 sagte in Codeteile auslagern. Bekomme cpplint Warnung:
...
(im Code von @DocShoe steht ja fälschlicherweiseservice_data
;- )
...Waaaas? Kann gar nicht sein!
Ergänzung zum Beitrag:
3) delete service_data versucht Speicher freizugeben, der unter dem Variablennamen überhaupt nicht allokiert wurde.
-
@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:
Epgsearch_lastconflictinfo_v1_0 *serviceData = new Epgsearch_lastconflictinfo_v1_0;
Das new ist noch da (1. Zeile).
Edit: Kommando zurück... Hab den korrigierten Code nicht gesehen... Teste das gleich mal...
Edit2: Klappt super! Vielen Dank und sorry, dass ich es nicht gleich gesehen habe
-
Dachte mir ich mach das bei cIndexFile auch so. Leider klappt das nicht:
g++ -Wall -g -O3 -Wall -Werror=overloaded-virtual -Wno-parentheses -fPIC -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -std=c++11 -c -DPLUGIN_NAME_I18N='"skinflatplus"' -DVDRLOGO=\"vdrlogo_yavdr\" -DWIDGETFOLDER='"/usr/lib/vdr/plugins/skinflatplus/widgets"' -I/usr/include/uuid -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/GraphicsMagick -o displayvolume.o displayvolume.c displayreplay.c: In member function ‘void cFlatDisplayReplay::UpdateInfo()’: displayreplay.c:335:29: error: cannot convert ‘cIndexFile’ to ‘cIndexFile*’ in assignment 335 | index = /*new*/ cIndexFile(recording->FileName(), false, recording->IsPesRecording()); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | cIndexFile make[1]: *** [Makefile:102: displayreplay.o] Fehler 1
Der Code sieht so aus:
cMarks marks; // From skinElchiHD - Avoid triggering index generation for recordings with empty/missing index bool hasMarks = false; cIndexFile *index = NULL; if (Recording->NumFrames() > 0) { hasMarks = marks.Load(Recording->FileName(), Recording->FramesPerSecond(), Recording->IsPesRecording()) && marks.Count(); index = new cIndexFile(Recording->FileName(), false, Recording->IsPesRecording()); }
Später wird dann auf index so zugegriffen:
if (hasMarks && index) { uint16_t FileNumber; off_t FileOffset; bool cutin = true; int32_t position {0}; cMark *mark = marks.First(); while (mark) { position = mark->Position(); index->Get(position, &FileNumber, &FileOffset);
-
@MegaV0lt Wenn du ein C++ Projekt pflegen willst, solltest du dir dringend Literatur anschaffen und lesen, oder zumindest mal irgendwo ein online Tutorial machen, so wird das echt mühselig.
cIndexFile *index = NULL;
index
Ist ein Pointer.new
gibt immer Pointer zurück.Wenn
cIndexFile
einen Ctor ohne Parameter anbietet, kannst du das versuchen:bool hasMarks = false; cIndexFile index; if (Recording->NumFrames() > 0) { hasMarks = marks.Load( Recording->FileName(), Recording->FramesPerSecond(), Recording->IsPesRecording()) && marks.Count(); index = cIndexFile(Recording->FileName(), false, Recording->IsPesRecording()); }
Den Zugriff auf Memberfunktionen musst du dann aber auch ändern:
index.Get(position, &FileNumber, &FileOffset);
-
Vielen Dank! Ctor geht nicht:
class cIndexFile { private: int f; cString fileName; int size, last; tIndexTs *index; bool isPesRecording; cResumeFile resumeFile; cIndexFileGenerator *indexFileGenerator; cMutex mutex; void ConvertFromPes(tIndexTs *IndexTs, int Count); void ConvertToPes(tIndexTs *IndexTs, int Count); bool CatchUp(int Index = -1); public: cIndexFile(const char *FileName, bool Record, bool IsPesRecording = false, bool PauseLive = false, bool Update = false); ~cIndexFile(); bool Ok(void) { return index != NULL; } bool Write(bool Independent, uint16_t FileNumber, off_t FileOffset); bool Get(int Index, uint16_t *FileNumber, off_t *FileOffset, bool *Independent = NULL, int *Length = NULL); int GetNextIFrame(int Index, bool Forward, uint16_t *FileNumber = NULL, off_t *FileOffset = NULL, int *Length = NULL); int GetClosestIFrame(int Index); ///< Returns the index of the I-frame that is closest to the given Index (or Index itself, ///< if it already points to an I-frame). Index may be any value, even outside the current ///< range of frame indexes. ///< If there is no actual index data available, 0 is returned. int Get(uint16_t FileNumber, off_t FileOffset); int Last(void) { CatchUp(); return last; } ///< Returns the index of the last entry in this file, or -1 if the file is empty. int GetResume(void) { return resumeFile.Read(); } bool StoreResume(int Index) { return resumeFile.Save(Index); } bool IsStillRecording(void); void Delete(void); static int GetLength(const char *FileName, bool IsPesRecording = false); ///< Calculates the recording length (number of frames) without actually reading the index file. ///< Returns -1 in case of error. static cString IndexFileName(const char *FileName, bool IsPesRecording); };
-
Generell gilt: Die Konstruktion eines Objekts erfordert immer einen passender Konstruktoraufruf. Die Parameter, die du bei der Konstruktion mit
new
übergibst, musst du auch bei der Konstruktion auf dem Stack angeben:// aus cIndexFile* index = new cIndexFile(Recording->FileName(), false, Recording->IsPesRecording()); // wird cIndexFile index(Recording->FileName(), false, Recording->IsPesRecording());
Ansonsten muss ich Schlangenmensch zustimmen, du solltest dir einen Kenntnisstand erarbeiten, mit dem du zumindest einfache Probleme selbst lösen kannst. Für jeden Compilerfehler einen Beitrag zu schreiben dauert ewig und vergrault Regs, die anfangs noch hilfsbereit sind.