Codeteile auslagern. Bekomme cpplint Warnung

MegaV0lt

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

Warum steht denn da eine "{" Klammer in der Header Datei?
Steht die Implementierung nicht in der Datei flat.c?
BTW: Sollte das nicht eine C++ Datei sein, also flat.cpp ?

1 Copy/Paste Fehler. In der .h steht es richtig - 1 Beitrag
2 Doch. steht in der .h
3. Das muss ich noch umbenennen. Ist alles altlast

Zu 3: Kann ich das mischen oder muss dann alles .cpp sein?
Hier mal das GIt zur Übersicht:
https://github.com/MegaV0lt/vdr-plugin-skinflatplus

Quiche Lorraine

Holla die Waldfee!

Dein Code sieht nicht gut aus. Ein paar Tipps:

Gebe alle C++ Dateien die Endung .cpp! Es ist, soweit ich weis, zwar nur eine Konvention, aber einige Programme interpretieren die Dateiendung!
Lade dir mal CppCheck herunter und versuche zu verstehen, über was das Programm meckert.
Warum nutzt du manuelle Speicherverwaltung? Ich zähle 276 new Anweisungen, 3 malloc Anweisungen, 24 delete Anweisungen und 2 free Anweisungen.
Achte bitte auf eine ggf. vorhandene Speicherverwaltung von einem SDK. Ich nutze z.B. cryptopp und da muss man stellenweise allokierte Daten mittels new übergeben und diese kümmert sich automatisch um die Freigabe. Gegebenenfalls würde ich solche Allokationen auch entsprechend dokumentieren (evt. über eine eigene spezielle new() Funktion, spezielle Typbezeichnungen,...)
Nutze verstärkt die STL, z.B. std::string statt char*.
Sei bitte vorsichtig mit der Verwendung von Pointern! Wenn du z.B. displayvolume.h anschaue, so verwendest du zwei Pointer. Was passiert aber wenn eine Instanz kopiert wird? Was passiert wenn das Objekt, auf welches der Pointer zeigt, ungültig (bzw. gelöscht) wird?
Lies dir diesbezüglich auch unbedingt die Rule of Five durch (https://en.cppreference.com/w/cpp/language/rule_of_three)!

Wo ist die Funktion GetComponents() definiert? Ich kann diese nicht finden!

PS:
Sorry übrigens dass ich dich so zerlegt habe.

MegaV0lt

Vielen Dank für's drüber schauen und zerpflücken. Ich habe das Projekt übernommen als der original Autor sich zurück gezogen hatte, um es am Leben zu halten. Ich bin Anfänger in dem Bereich und bastel mehr oder weniger dran Rum. Hab schon ein paar Sachen gemacht.
Das Plugin ist schon mintestens zehn Jahre alt. Da sind viele "Altlasten" drin.

Die Tipps sind sehr willkommen.

Ich werde sie auf meine Liste setzen und versuchen umzusetzen.

Dateien umbenennen kann ich versuchen. Muss aber wohl das Makefile anpassen und schauen, ob es auch in anderen Distributionen gebaut werden kann...

cppcheck bringt eigentlich keine Fehlermeldungen (--language=c++)

Das mit der Speicherverwaltung ist mir noch gar nicht aufgefallen.

Was ist der Grund Die STL zu bevorzugen? Ich versuche eher das cSting vom VDR zu verwenden.

Die beiden Pointer in displayvolume beziehen sich auf PixMaps, dei verwendet werden um den Lautstärkebalken und den Wert und eventuel das mute Symbol anzuzeigen. Da verstehe ich die Bedenken nicht so richtig.

Rule of Five werde ich mir noch anschauen...

Zu der Funktion: Die ist noch nicht im GIT, da ich sie erst im Live-Betrieb am VDR testen möchte. Mir passieren leider viel zu Oft Fehler. Bin halt kein Programmierer.

Noch mal Danke fürs schauen und kommentieren. Kommentare oder Kritik ist immer gerne gesehen. Man will ja besser werden. Und ohne Hilfe bekomm ich das auch gar nicht hin

DocShoe

@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Was ist der Grund Die STL zu bevorzugen? Ich versuche eher das cSting vom VDR zu verwenden.

STL steht für Standard Template Library. Das "Standard" darin sollte Grund genug sein, besser Klassen aus ihr zu benutzen, statt proprietärer Alternativen.

Schlangenmensch

@MegaV0lt Ich würde nochmal gerne kurz auf deine Ursprüngliche Frage eingehen wollen.
@Th69 hat ja bereits geschrieben, dass die Warnung aufgrund einesr älteren Google C++ Style Guide ausgegeben wird.

Ich persönlich habe auch Stellen bei mir im Code, an denen ich Referenzen als Out Parameter verwende.

Man könnte sich überlegen, warum das mal in dem Coding Guideline drinnen stand.

Das Problem bei Referenzen ist, du siehst es dem Aufruf der Funktion nicht direkt an, dass die Werte verändert werden. Du hast also irgendwie eine GetComponents Funktion, die nicht etwa eine Komponente zurück gibt, sondern die Strings Text, Audio und Subtitle verändern. Ich würde sagen, dass ist für die Funktion, rein vom Titel her, nicht erwartetes Verhalten.

Man könnte, neben eine Umbennung, z.B. überlegen die Strings als Kopie zu übergeben (ok, kann teuer sein) und dann ein Tuple von Strings zurück zu geben. Dann sieht man an der Benutzung des Codes direkt, was da passiert.
Bzw. zumindest Audio und Subtitle wird ja erst vor der Funktion deklariert. Dann könnte man den String direkt in der Funktion anlegen und über den Tuple zurück geben. Dann benötigt man den Parameter gar nicht.

@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Was ist der Grund Die STL zu bevorzugen? Ich versuche eher das cSting vom VDR zu verwenden

Die STL wird an vielen Stellen eingesetzt und ist daher super getestet. Außerdem bietet sie ein weites Spektrum an Algorithmen an, mit denen man viele Probleme elegant und einfach lösen kann.

MegaV0lt

Da wäre ein Name wie

InsertComponents()

wohl besser. Ich werde das gleich machen. PS: Die Funktion ist jetzt auch im GIT

Th69

Dann wohl eher GetComponentsTexts - du gibst ja keine Komponenten zurück oder fügst welche ein.

*john 0

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

Gebe alle C++ Dateien die Endung .cpp! Es ist, soweit ich weis, zwar nur eine Konvention, aber einige Programme interpretieren die Dateiendung!

Weil Programme die Endungen interpretieren, sollte man auf UNIX/Linux definitiv nicht .cpp nutzen! Das Problem ist hierbei, dass auf UNIX früher das Programm cpp existierte, welches C-Quelldateien vorverarbeitete (Präprozessor) und dann den expandierten Quellcode in .cpp Dateien ablegte, welche vom cc in .o Übersetzt wurden. Daher interpretieren noch so einige Programme .cpp nicht als C++ Sourcecode.

Es gibt eine Reihe von Dateiendungen, die sich für C++-Quelldateien etabliert haben: .C, .cc, .cxx, .cpp sind die gebräuchlichsten. .C ist von Stroustrup persönlich verwendet worden, und bereitet so ziemlich jeden auf DOS/Windows Freude. Für portable Programme ist daher .cc ~~oder .cxx~~ sinnvoller, da man weder auf der einen noch auf der anderen Plattform Probleme zu erwarten hat.

wob

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

Daher interpretieren noch so einige Programme .cpp nicht als C++ Sourcecode.

Ich hatte mich tatsächlich auch schon öfter mal gefragt, warum ich so viel .cc oder .cxx gesehen habe, aber nur selten .cpp. Das erklärt es!

Aus Interesse: hast du Beispiele für Programme, die .cpp anders interpretieren?

Quiche Lorraine

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

Das Problem ist hierbei, dass auf UNIX früher das Programm cpp existierte

Ups, habe gerade auf meiner Manjaro Installation das Programm cpp entdeckt.

Linux hat irgentwie lustige Befehle wie cpp, wc, tee.

*john 0

@wob sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Aus Interesse: hast du Beispiele für Programme, die .cpp anders interpretieren?

Ein heißer Kandidat dafür sind die original UNIX makes und auch ältere GNU make Versionen.

Ich habe gerade OpenIndiana installiert, und auch die neuste Version hat ein make was nur .C und .cc in den implicit rules für C++ Sourcecode kennt. Man kann entweder ein aktuelles GNU make nachinstallieren, oder man definiert explizite Regeln, um dieses „Problem“ zu umgehen. Dennoch sollte man nicht erwarten, dass diese Dateiendung korrekt out-of-the-box erkannt wird.

firefly

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

@wob sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Aus Interesse: hast du Beispiele für Programme, die .cpp anders interpretieren?

Ein heißer Kandidat dafür sind die original UNIX makes und auch ältere GNU make Versionen.

Ich habe gerade OpenIndiana installiert, und auch die neuste Version hat ein make was nur .C und .cc in den imlicit rules für C++ Sourcecode kennt. Man kann entweder ein aktuelles GNU make nachinstallieren, oder man definiert explizite Regeln, um dieses „Problem“ zu umgehen. Dennoch sollte man nicht erwarten, dass diese Dateiendung korrekt out-of-the-box erkannt wird.

Da würde dann auch *.cxx rausfallen aus deinem "Vorschlag" was portabler sei zu nutzen

Wobei die Frage ist, wie relevant das noch ist. Wie viele C++ Projekte nutzen direkt Makefiles?
Soweit ich das mitbekommen habe, sind in laufe der Zeit viele Projekte von automake/autotools auf cmake und co gewechselt. Besonders da diese tools deutlich mehr OS/Platformen unterstützen als automake/autotools.
Zusätzlich wird vermehrt statt (gnu) make das build tool ninja verwendet.

Eines der umfangreichsten C++ Projekte, die mir bekannt sind, ist das LLVM projekt.
Und das verwendet für seinen C++ code die Endung .cpp und nutzt cmake.

Daher sehe ich das aktuell das ganze eher als ein geringeres Problem an, die Dateiendung .cpp für C++ Sourcecode zu nutzen.

*john 0

@firefly sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Wobei die Frage ist, wie relevant das noch ist. Wie viele C++ Projekte nutzen direkt Makefiles?

Viele, und der Punkt ist, dass man den Raum der Möglichkeiten einschränkt, ohne dass es dafür einen gewichtigen Grund gibt – außer Gewohnheit. Google legt im eigenem Style Guide sich auch auf .cc für C++ Sourcedateien fest.

Es geht nicht nur um make sondern darum, dass es auf einigen Plattformen noch immer wichtige Werkzeuge gibt, die .cpp nicht als C++ Source erkennen. Das UNIX make von OpenIndiana reicht somit als Beispiel aus, um .cpp nicht als Empfehlung für die Dateiendung zu geben. Und es ist auch nicht entscheidend, dass man Workarounds für das Problem findet.

firefly

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

@firefly sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Wobei die Frage ist, wie relevant das noch ist. Wie viele C++ Projekte nutzen direkt Makefiles?

Viele, und der Punkt ist, dass man den Raum der Möglichkeiten einschränkt, ohne dass es dafür einen gewichtigen Grund gibt – außer Gewohnheit. Google legt im eigenem Style Guide sich auch auf .cc für C++ Sourcedateien fest.

Es geht nicht nur um make sondern darum, dass es auf einigen Plattformen noch immer wichtige Werkzeuge gibt, die .cpp nicht als C++ Source erkennen. Das UNIX make von OpenIndiana reicht somit als Beispiel aus, um .cpp nicht als Empfehlung für die Dateiendung zu geben. Und es ist auch nicht entscheidend, dass man Workarounds für das Problem findet.

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.

Und wiso soll die Nutzung von anderen build tools als "Nutzung von Workarounds" angesehen werden?
Dieser Argument ist für mich nicht verständlich.
Denn die Wahl das anderer build tools genutzt werden, wird meist dadurch getroffen, dass diese einen vorteil bieten.
Sei es eine einfachere/verständlichere Syntax oder zusätzliche Features.
Und eher weniger dass diese irgendwelche angeblichen Workarounds umsetzen.

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

Aber das ganze trifft jetzt eher in Bereiche ab, welche für die meisten nicht von relevanz sind.

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