Vereinfachung möglich und sinnvoll?

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

if ((uchar)*s > ' ') // most strings don't have any leading space, so handle this case as fast as possible
return (char *)s;

early exit : Der String enthält keine führenden Leerzeichen, die weitere Verarbeitung kann sofort beendet werden

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

while (*s && (uchar)*s <= ' ') // avoiding isspace() here, because it is much slower
s++;

Iteration : Der char pointer wird so lange weitergesetzt, bis das Ende oder das erste Zeichen erreicht wurde, das kein Leerzeichen darstellt - dabei wird die Funktion isspace() vermieden und stattdessen händisch überprüft

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

return (char *)s;

regular exit : Der übriggebliebene String wird zurückgegeben (Pointer darauf)

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

inline char *skipspace(const char *s)

Anzumerken ist hier vielleicht noch ... dass die Funktion mit der Signatur (const char const *s) nicht funktionieren würde, weil dann der char-Pointer s auch nicht modifiziert werden dürfte ...

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Das lässt sich doch bestimmt vereinfachen,

Wie es @SeppJ bereits gesagt hatte ... Deine Funktion tut insgesamt zu viel ... In einem ersten Schritt könntest Du diese in kleinere aufteilen, um dann zu sehen, was möglicherweise "wegkann".

Ich habe Deine Funktion nicht im Detail nachvollzogen.

MegaV0lt

@SeppJ
Zwei Fälle, da ein Plugin es anders darstellt. Möglich sind:

"Eine Serie~Folge 1"
"Eine Serie ~ Folge 1"

Beide Strings sollen in
"Eine Serie" und "Folge 1" gesplittet werden. Im OSD wird der zweite String in einer anderen Farbe angezeigt.

Die Farrbe ist einstellbar und auch abschaltbar (if (ColorExtraTextFg))

firefly

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

@SeppJ
Zwei Fälle, da ein Plugin es anders darstellt. Möglich sind:

"Eine Serie~Folge 1"
"Eine Serie ~ Folge 1"

Beide Strings sollen in
"Eine Serie" und "Folge 1" gesplittet werden. Im OSD wird der zweite String in einer anderen Farbe angezeigt.

Die Farrbe ist einstellbar und auch abschaltbar (if (ColorExtraTextFg))

Wäre das nicht aus lösbar mit

Splitte string mit trennzeichen '~'
Trimme die strings von leerzeichen zu beginn/ende
Dann wäre das für beide fälle verwendbar ohne separaten code zu benötigen

meuterei

Oder mit regex, das ginge auch, um beide zu berücksichtigen...

MegaV0lt

Ich habe das im Netz gefunden

// trim from left
inline std::string &ltrim(std::string &s, const char* t = " \t\n\r\f\v") {
    s.erase(0, s.find_first_not_of(t));
    return s;
}

// trim from right
inline std::string& rtrim(std::string& s, const char* t = " \t\n\r\f\v") {
    s.erase(s.find_last_not_of(t) + 1);
    return s;
}

Und der Code sieht jetzt so aus:

    if (ColorExtraTextFg) {
        std::string tilde = Text;
        size_t found = tilde.find('~');  // Search for ~
        if (found != std::string::npos) {
            std::string first = tilde.substr(0, found);
            std::string second = tilde.substr(found + 1, tilde.length());
            rtrim(first);   // Trim possible space on right side
            ltrim(second);  // Trim possible space at begin

            Pixmap->DrawText(cPoint(0, 0), first.c_str(), ColorFg, ColorBg, Font);
            int l = Font->Width(first.c_str());
            l += Font->Width('X');
            Pixmap->DrawText(cPoint(l, 0), second.c_str(), ColorExtraTextFg, ColorBg, Font);
        } else  // ~ not found
            Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    } else {  // No extracolor defined
        Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    }

Schon viel kleiner...
Nur von cpplint kommt ne Warnung:

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

Das kann ich nicht lösen

SeppJ

Schlechte Warnung, wo dir jemand seinen fragwürdigen, restriktiven Stil aufzwingen will. Ja, es ist mit Absicht eine non-const Referenz. Das kann schlechter Stil sein, wenn man es versteckt an unerwarteten Orten, aber hier ist das intuitiv klar und erwartet, dass die Funktion so benutzt werden sollte.

MegaV0lt

Warum wird eigentlich noch ein

return s;

benötigt? Und stimmt der Aufruf mit

ltrim(second);

Wird da nur die Referenz übergeben...

SeppJ

Nötig ist es nicht, aber es ermöglicht auch die Nutzung in verketteten Aufrufen, a la

cout << rtrim(my_string);

Wobei wir da tatsächlich so langsam in Gebiete kommen, wo die non-const Referenz zu gefährlichen Irrtümern führen kann. Beispielsweise wäre

result = ltrim(my_string)+ rtrim(my_string)

nicht so günstig.

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Nur von cpplint kommt ne Warnung:
Is this a non-const reference? If so, make const or use a pointer: std::string &s cpplint(warning:runtime/references)
Das kann ich nicht lösen

Vermutlich könnte der zweite Parameter auch so deklariert werden:

const char * const t

Aber das ist, wie es SeppJ erklärt hat, gewissermaßen unnötig erzwungen, würde ich sagen.

*john 0

@SeppJ sagte in Vereinfachung möglich und sinnvoll?:

Nötig ist es nicht, aber es ermöglicht auch die Nutzung in verketteten Aufrufen, a la
cout << rtrim(my_string);
Wobei wir da tatsächlich so langsam in Gebiete kommen, wo die non-const Referenz zu gefährlichen Irrtümern führen kann. Beispielsweise wäre
result = ltrim(my_string)+ rtrim(my_string)
nicht so günstig.

Persönlich finde ich das besser entweder

std::string trim (const std::string& input);

oder

void trim (std::string& input_output);

zu wählen.

Ich vermisse mittlerweile in C++ ein Fortran Feature – die intent Deklaration von Parametern in Funktionen und Subroutinen. Funktionen können übrigens nur „in“ Parameter haben, Subroutinen „in“, „out“ und „in out“.

SeppJ

Ja, ich kann durchaus die Meldung des Linters da verstehen. Komplett neue Programmierer können sich da durchaus auf die Schnauze legen, und ein bösartiger Programmierer könnte absichtlich auch erfahrenen Programmierern ein Bein stellen. Aber ich finde es stark übertrieben. Die Hürde ist jetzt echt nicht hoch, das zu lernen. Und wenn du bösartige Feinde hast, werden die dich auf andere Weise kriegen. Ein bisschen Niveau muss man voraussetzen dürfen, sonst kommt man zu nix. Dieser Linter würde die gleiche Diagnose für einen großen Teil der Standardbibliothek liefern müssen.

DocShoe

Die boost-Stringbibliotheken bennenen ihre trim() Funktion explizit, da gibt es Sachen wie trim( std::string& s ) und trim_copy( std::string const& s ), in allen Kombinationen (left, right mit und ohne copy im Namen). Sieht manchmal etwas sperrig aus, aber dafür sieht man bei

std::string const cleaned = trim_copy( input );

oder

std::string const cleaned2 = trim_left_copy( input );

aber auch sofort, was da passiert.

Finnegan

Dieses ganze trim, find und substr sieht mir so aus, als ginge es im Prinzip nur darum, den "richtigen" Substring in einem vorhandenen String zu finden. Da kann man eventuell auch überlegen, mit std::string_view zu arbeiten. So ließen sich einige Kopier- und Speicherallokations-Operationen einsparen... aber gut möglich, dass das bei diesem Code nur wenig ins Gewicht fällt und daher nicht so wichtig ist.

Das sieht mir aber durchaus wie eine Rendering-Funktion aus, da lassen sich solche Optimierungen meistens schon rechtfertigen.

meuterei

@DocShoe sagte in Vereinfachung möglich und sinnvoll?:

mit und ohne copy im Namen

Das gefällt mir. +1

Ich bin mittel-fortgeschrittener Anfänger, und für mich ist es eine Erleichterung, die Funktionsweise nicht über konstante bzw. nicht konstante Funktionsparameterdeklarationen "erahnen" zu müssen ... sondern sie direkt am Namen der Funktion ablesen zu können.

MegaV0lt

Ich habe das jetz so abgeändert, da ich das trim nur an den besagten Stellen brauche:

// Trim from left
inline void ltrim(std::string &s, const char* t = " \t\n\r\f\v") {
    s.erase(0, s.find_first_not_of(t));
}

// Trim from right
inline void rtrim(std::string& s, const char* t = " \t\n\r\f\v") {
    s.erase(s.find_last_not_of(t) + 1);
}

Es wäre auch ein Name im Stil von

inplace_rtrim()

denkbat

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

find_first_not_of

Heißt das nicht not_in anstatt not_of? In SQL heißt's so... Oder not_one_of?

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Es wäre auch ein Name im Stil von
inplace_rtrim()

Th69

Und ob die Antwort richtig ist, verrät dir jetzt das Licht: std::basic_string<CharT,Traits,Allocator>::find_first_not_of

meuterei

@Th69 sagte in Vereinfachung möglich und sinnvoll?:

Und ob die Antwort richtig ist, verrät dir jetzt das Licht: std::basic_string<CharT,Traits,Allocator>::find_first_not_of

If all characters in the range can be found in the given character sequence, npos will be returned.

Ich würde sagen ... die Namensgebung ist an der Stelle einfach nicht folgerichtig.