Re: [Lost] Neue io_get_absolute_path

[Toni Kaufmann <toni@xxxxxxxxxxxxxxxx>]

Kevin Wolf wrote:
>> +/**
>> + * Zerstueckelt einen Pfad in die einzelnen Elemente
>> + */
>> +static bool get_path_element(const char* path, int index, struct path_element* element)
>>     
>
> Soll der Kommentar eine ausreichende Beschreibung für eine Funktion mit 
> diesem Prototyp sein? Ich kann mir darunter ehrlich gesagt nichts 
> vorstellen, ohne den Code anzuschauen. Beschreibung der Parameter und 
> des Rückgabewerts wäre nett - bei den folgenden Funktionen ebenso.
>   
Ok stimmt, da war ich etwas geizig mit dem Kommentar ;-)
>> +{
>> +    bool escaped = FALSE;
>> +    char* pos = (char*) path;
>> +    char* last_element = (char*) path;
>> +    path_sep_t left_sep = NO_SEP;
>> +
>> +    // Pfad Zeichenweise durchgehen
>> +    while (TRUE) {
>> +        // Wenn das letzte Zeichen ein Escape-Zeichen war, muss das aktuelle
>> +        // nicht beruecksichtigt werden
>> +        if (escaped == TRUE) {
>> +            escaped = FALSE;
>> +        } else {
>> +            size_t cur_size = (uintptr_t) pos - (uintptr_t) last_element;
>> +            // Wenn das aktuelle Zeichen ein Escape ist, wird escaped auf TRUE
>> +            // gesetzt, damit das naechste Zeichen nicht beachtet wird
>> +            if (*pos == '\\') {
>>     
> Darf ich mir hier ein switch wünschen?
>   
Und darf ich fragen was du gerne geswitcht sehen würdest? Respektiv wie
du dir den 2. Block mit dem :/ vorstellst? Mit einem case aussen und
noch einem if drin?
>> +                escaped = TRUE;
>> +            }
>> +            // Auf Service-Seperator :/ Pruefen
>> +            else if ((*pos == ':') && (*(pos + 1) == '/')) {
>> +                // Ist das das gewuenschte Element?
>> +                if (index-- == 0) {
>> +                    element->left_sep = left_sep;
>> +                    element->right_sep = SERVICE_SEP;
>> +                    
>> +                    // Text kopieren
>> +                    memcpy(element->text, last_element, cur_size);
>> +                    element->text[cur_size] = '\0';
>> +
>> +                    // Wir sind am Ende angekommen
>> +                    return TRUE;
>> +                }
>>     
>
> Dieses if kommt ein paar Mal unverändert vor und Codeduplikation ist 
> böse. Das memcpy könnte außerdem element->text overflowen, wenn ich mich 
> nicht täusche und path lang genug ist.
>   
Okay, ich werde mal schauen was ich machen kann...
Wie soll ich denn das mit dem text deiner Meinung nach machen?
Allozieren? Oder einfach abschneiden?
>> +            // Pfad-Seperator /
>> +            else if (*pos == '|') {
>>     
>
> Offensichtlich unverändert kopierter Kommentar.
>   
Auf gar keinen Fall... das ist nicht so wies aussieht *g*
>> +/**
>> + * Erstellt aus einem Beliebigen Pfad einen absoluten
>> + */
>> +char* io_get_absolute_path(const char* path)
>>     
>
> Vielleicht besser einen Puffer für das Ergebnis mitübergeben?
>
>   
Das habe ich mir auch schon überlegt, aber der Aufrufer weiss ja nicht
wie gross das Ding wird. Deswegen habe ich das so gemacht.
>> +{
>> +    char* cwd = getcwd(NULL, 0);
>> +    // Hier wird eine Liste mit all den Pfad-Elementen erstellt
>> +    struct path_element* element = malloc(sizeof(struct path_element));
>> +    list_t* element_stack = list_create();
>> +
>> +    int i = 0;
>> +    // Elemente einzeln holen, und an Liste anhaengen
>> +    while (get_path_element(path, i++, element) == TRUE) { 
>> +        list_push(element_stack, element);
>> +        element = malloc(sizeof(struct path_element));
>> +    }
>> +    // Das letzte Element ist unbenutzt und kann freigegeben werden
>> +    free(element);
>>     
>
> Das Verfahren funktioniert zwar, ist aber natürlich völlig ineffizient. 
> Du nimmt den Pfad hier in O(n²) auseinander, indem du für jeden 
> Bestandteil den kompletten String neu parst.
>
>   
Jo, ist mir bewusst. Ich glaube ich Ändere die Funktion oben so ab, dass
sie Alle Elemente aufs mal in eine Liste hängt....
>> +    // Jetzt werden relative Pfade aufgeloest
>> +    int last_element = list_size(element_stack) - 1;
>> +    element = list_get_element_at(element_stack, last_element++);
>>     
>
> Das ist verwirrend. Wozu erst eins abziehen und in der nächsten Zeile 
> wieder ein verstecktes ++ drauf? Außerdem wäre ein Hinweis gut, daß 
> last_element logisch das erste (vorderste) Element bezeichnet.
>   
Werde ich deutlicher beschreiben