Codeteile auslagern. Bekomme cpplint Warnung

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.

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