Codeteile auslagern. Bekomme cpplint Warnung

*john 0

@firefly sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Dann solltest du auch mal deine eigenen Vorschlag "was sinvoller sei" mal überarbeiten, denn dort hast du .cxx angegeben zu nutzen aber das funktioniert mit der unix make von OpenIndiana auch nicht.

Leider spielt einem manchmal das Gedächtnis einen Streich. Als ich diesen Text schrieb hatte ich noch nicht nachgeschaut, was wirklich möglich ist bzw. früher war. Persönlich verwende ich seit langer Zeit nur .cc, weil das die wenigsten Probleme macht. Ich habe nicht mehr alle Eigenheiten der diversen Toolchains im Kopf, nur das Endresultat, dass .cc am sinnvollsten ist.

Und wiso soll die Nutzung von anderen build tools als "Nutzung von Workarounds" angesehen werden?

Nein, das meinte ich mit Workaround nicht sondern den Umstand, dass man Compiler meist mit einem speziellem Flag mitteilen kann, dass es sich doch um C++ Sourcecode handelt oder für make kann man explizite Regeln definieren. Denn oftmals ist die Toolchain vorgeschrieben und man kann eben nicht frei wählen.

Denn die Wahl das anderer build tools genutzt werden, wird meist dadurch getroffen, dass diese einen vorteil bieten.

Absolut, aber niemand wird Dir eine anderes Buildtool erlauben, weil Du Deine Sourcen falsch benannt hast.

Und welchen "Raum der Möglichkeiten" wird da angeblich eingeschänkt, wenn man nicht direkt Makefiles nutzt?

Es geht nicht um die Makefiles, es geht um die Dateiendungen über die wir hier die ganze Zeit diskutieren.

firefly

@john-0 sagte in Codeteile auslagern. Bekomme cpplint Warnung:

@firefly sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Und welchen "Raum der Möglichkeiten" wird da angeblich eingeschänkt, wenn man nicht direkt Makefiles nutzt?

Es geht nicht um die Makefiles, es geht um die Dateiendungen über die wir hier die ganze Zeit diskutieren.

Nur das du deine Antwort auf meine Frage gebracht hast "Wie viele C++ Projekte nutzen direkt Makefiles?"
Und sich die Antwort auch klar auf diese Frage bezog.
Daher hat sich da das Thema aus meiner Sicht auf Makefiles verschoben.
Wodurch es dann kein wunder ist, wenn beim gegenüber die Frage aufkommt wieso die nicht direkte Nutzung von Makefiles "den Raum der Möglichkeiten einschränken" soll.

Unabhängig davon ist das gut zu wissen, dass es Platformen/tools gibt wo es mit .cpp als Dateiendung Probleme geben kann.
Wobei das nur soweit relevant ist, wenn man mit solchen Platformen arbeitet.
Denn die meist genutzten Platformen, die mir bekannt sind (Windows und Linux), haben dieses Problem nicht.
Und selbst für dein OpenIndiana Argument gibt es mit dem LLVM Projekt ein Beispiel, welches zeigt, dass das Problem nicht so gravierend ist, wie du es (aus meiner Sicht) darstellen möchtest.

Denn in OpenIndiana Hipster 2023.10 ist LLVM/Clang 17 enthalten. (https://www.openindiana.org/announcements/openindiana-hipster-2023-10-announcement/)

Absolut, aber niemand wird Dir eine anderes Buildtool erlauben, weil Du Deine Sourcen falsch benannt hast.

Was ich auch nicht behauptet habe, schön wie hier einfach sachen rein interpretiert werden die nie gesagt wurden...

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