Codeteile auslagern. Bekomme cpplint Warnung

MegaV0lt

@Quiche-Lorraine sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Warum nutzt du manuelle Speicherverwaltung? Ich zähle 276 new Anweisungen, 3 malloc Anweisungen, 24 delete Anweisungen und 2 free Anweisungen

Ein paar habe ich schon beseitigen können. Leider fehlen mir für mehr die Kenntnisse.
Habe noch ein malloc:

    char *p = (char *) malloc(hor_filters_size + ver_filters_size + buffer_size);

in imagescaler.c
und einige new. Beispiele:

if (p) {
                Epgsearch_lastconflictinfo_v1_0 *serviceData = new Epgsearch_lastconflictinfo_v1_0;
                if (serviceData) {
                    serviceData->nextConflict = 0;
                    serviceData->relevantConflicts = 0;
                    serviceData->totalConflicts = 0;
                    p->Service("Epgsearch-lastconflictinfo-v1.0", serviceData);
                    if (serviceData->relevantConflicts > 0) {
                        numConflicts = serviceData->relevantConflicts;
                    }
                    delete serviceData;
                }
            }

            if (Recording->NumFrames() > 0) {
                hasMarks =
                    marks.Load(Recording->FileName(), Recording->FramesPerSecond(), Recording->IsPesRecording()) &&
                    marks.Count();
                index = new cIndexFile(Recording->FileName(), false, Recording->IsPesRecording());
            }

    // Check currently recording devices
    bool *recDevices = new bool[numDevices];
    for (int i {0}; i < numDevices; ++i)
...

Einige von der Sorte

cSkinDisplayChannel *cFlat::DisplayChannel(bool WithInfo) {
    return new cFlatDisplayChannel(WithInfo);
}

cOsdItem *cMenuSetupSubMenu::InfoItem(const char *label, const char *value) {
    cOsdItem *item;
    item = new cOsdItem(cString::sprintf("%s: %s", label, value));
    item->SetSelectable(false);
    return item;
}

Keine Ahnung, wie man das alles ohne new macht.

DocShoe

@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 und FlatDisplayChannel statt cOsdItem und cFlatDisplayChannel.
Warum benutzt du const char* für Zeichenketten statt std::string?
Für fast alle Arrayallokationen kann std::vector benutzt werden. Vorsicht bei std::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 oder malloc 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

wob

@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++.

firefly

@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.

DocShoe

@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?

MegaV0lt

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

MegaV0lt

@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);
};

*john 0

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.

Schlangenmensch

@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

@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

firefly

@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.

DocShoe

@MegaV0lt

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

MegaV0lt

@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?

Th69

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 die Service-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 kein delete serviceData (im Code von @DocShoe steht ja fälschlicherweise service_data ;- ) stehen.

DocShoe

@Th69 sagte in Codeteile auslagern. Bekomme cpplint Warnung:

...
(im Code von @DocShoe steht ja fälschlicherweise service_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

@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

MegaV0lt

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);

Schlangenmensch

@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);

MegaV0lt

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);
  };

DocShoe

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.