Codeteile auslagern. Bekomme cpplint Warnung

MegaV0lt

Folgender Code wird vier mal verwendet. Ich wollte ihn darum in eine Funktion auslagern. Hier mal die Funktion (flat.c):

void GetComponents(const cComponents *Components, cString &Text, cString &Audio, cString &Subtitle) {
    bool firstAudio = true, firstSubtitle = true;
    const char *audio_type = NULL;
    for (int i {0}; i < Components->NumComponents(); ++i) {
        const tComponent *p = Components->Component(i);
        switch (p->stream) {
        case sc_video_MPEG2:
            if (p->description)
                Text.Append(cString::sprintf("\n%s: %s (MPEG2)", tr("Video"), p->description));
            else
                Text.Append(cString::sprintf("\n%s: MPEG2", tr("Video")));
            break;
        case sc_video_H264_AVC:
            if (p->description)
                Text.Append(cString::sprintf("\n%s: %s (H.264)", tr("Video"), p->description));
            else
                Text.Append(cString::sprintf("\n%s: H.264", tr("Video")));
            break;
        case sc_video_H265_HEVC:  // Might be not always correct because stream_content_ext (must be 0x0) is
                                  // not available in tComponent
            if (p->description)
                Text.Append(cString::sprintf("\n%s: %s (H.265)", tr("Video"), p->description));
            else
                Text.Append(cString::sprintf("\n%s: H.265", tr("Video")));
            break;
        case sc_audio_MP2:
        case sc_audio_AC3:
        case sc_audio_HEAAC:
            if (firstAudio)
                firstAudio = false;
            else
                Audio.Append(", ");
            switch (p->stream) {
            case sc_audio_MP2:
                if (p->type == 5)  // Workaround for wrongfully used stream type X 02 05 for AC3
                    audio_type = "AC3";
                else
                    audio_type = "MP2";
                break;
            case sc_audio_AC3: audio_type = "AC3"; break;
            case sc_audio_HEAAC: audio_type = "HEAAC"; break;
            }
            if (p->description)
                Audio.Append(cString::sprintf("%s (%s)", p->description, p->language));
            else
                Audio.Append(cString::sprintf("%s (%s)", p->language, audio_type));
            break;
        case sc_subtitle:
            if (firstSubtitle)
                firstSubtitle = false;
            else
                Subtitle.Append(", ");
            if (p->description)
                Subtitle.Append(cString::sprintf("%s (%s)"), p->description, p->language);
            else
                Subtitle.Append(p->language);
            break;
        }  // switch
    }  // for
}

flat.h:

void GetComponents(const cComponents *Components, cString &Text, cString &Audio, cString &Subtitle);

Und der Aufruf im Code (displaymenu.c):

            const cComponents *Components = Event->Components();
            if (Components) {
                cString Audio(""), Subtitle("");
                GetComponents(Components, Text, Audio, Subtitle);  // Get info for audio/video and subtitle

Von cpplint bekomme ich eine Warnung:

Is this a non-const reference? If so, make const or use a pointer: cString &Text cpplint(warning:runtime/references)

Analog auch zu &Audio und &Subtitle

Warum sollen hier Pointer verwendet werden? Wenn ich Pointer verwende, müsste es ja auch so gehen?

Th69

Bei dem Funktionsaufruf wird der Referenz-/Adressoperator nicht angegeben:

GetComponents(Components, Text, Audio, Subtitle);

MegaV0lt

@Th69 Vielen Dank. das ändert aber nicht die Warn-Meldug in der .h Datei

void GetComponents(const cComponents *Components, cString &Text, cString &Audio, cString &Subtitle);

const cComponents *Components
------------------^

ist vermutlich ein Schreibfehler, will der Compiler sagen ...

Quiche Lorraine

@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Vielen Dank. das ändert aber nicht die Warn-Meldug in der .h Datei
void GetComponents(const cComponents *Components, cString &Text, cString &Audio, cString &Subtitle) {
...

Ist das die Definition des Headers?

Wenn du die Funktion nicht komplett in die .h Datei geschrieben hast, würde ich mal die Headerdefinition mit der Implementierung exakt vergleichen.

Th69

Die cpplint-Warnung ist aufgrund einer älteren Empfehlung von dem Google C++ Style Guide (sie empfahlen dort dann explizit Zeiger zu verwenden, um anzuzeigen, daß der Parameter verändert wird), s.a. Confusion over cpplint warning, "Is this a non-const reference? If so, make const or use a pointer:" (falls du englisch kannst).
Du kannst dies wohl mit NOLINT(runtime/references) deaktivieren.

@NoIDE: Nein, wie kommst du drauf?

Edit: @MegaV0lt: Evtl. mal den 'fork' cpplint/cpplint benutzen?

@Th69 sagte in Codeteile auslagern. Bekomme cpplint Warnung:

Nein, wie kommst du drauf?

Es macht doch keinen Sinn, eine Kopie eines konstanten Zeigers zu übergeben. Aber wahrscheinlich hab ich nicht korrekt gelesen.

MegaV0lt

Hab mal alles im ersten Beitrag nachgetragen.
Die Funktion ist in der flat.c
Header in der flat.h
Aufruf erfolg aus der displaymenu.c (Vier mal)

Th69

@MegaV0lt: Welche genaue 'cpplint'-Version verwendest du denn?

@NoIDE: Nicht der Zeiger ist konstant, sondern es ist ein Zeiger auf konstante (also unveränderliche) Objekte!
const cComponents *Components ist das gleiche wie cComponents const *Components.
Was du meinst wäre cComponents * const Components (oder const cComponents * const Components). Aber selbst dann könnte man den Zeiger problemlos übergeben.

PS: @MegaV0lt, ich wollte mal nachfragen, ob dir unsere Codeänderungen in if-Abfrage Vereinfachung geholfen haben - hast dich bisher nicht mehr zurückgemeldet.

MegaV0lt

@Th69 Ich habe das so gelassen wie ich es geschrieben habe. Funktioniert.

cpplint ist
Cpplint fork (https://github.com/cpplint/cpplint)
cpplint 1.5.5
Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]

Th69

Die Funktion CheckForNonConstReference ist in der aktuellen Version 1.6.1 wohl immer noch drin.

Im Python-Quellcode Zeile 101 ff. (bzw. im "Usage"-Text) steht, daß es auch noch NOLINTNEXTLINE(...) gibt (je nachdem wo du den Kommentar hinschreibst).

Quiche Lorraine

@MegaV0lt sagte in Codeteile auslagern. Bekomme cpplint Warnung:

void GetComponents(const cComponents *Components, cString &Text, cString &Audio, cString &Subtitle) {

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 ?

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.