4. Dynamisches Array von Pointern/Adressen in einem struct

Dynamisches Array von Pointern/Adressen in einem struct

  • T

    Hallo EOP,

    ich teste den Code mit Valkyrie bzw. ist das ein Frontend für Valgrind/Memcheck unter Linux/Debian.

    Aller Speicher wurde korrekt freigegeben. Aber du meinst vermutlich das ständige Malloc, Realloc's ?

    Ich habe zum Großteil schon darauf geachtet und getestet das Speicher korrekt freigegeben wird, ob das bei dem Problem eine Rolle spielt, ich weiß es nicht.

    Aber so lange man der Aussage von Valgrind trauen darf, wird kein Byte geleaked oder ein Byte geschrieben das nicht reserviert wurde oder ein Byte gelesen das nicht geschrieben wurde.

    Wie es mit dem Pointer zu Pointern/Adressen aussieht, das habe ich nicht nachvollzogen.

    *Edit:
    In der Tat leaked der Code gerade ein paar Bytes, ich habe jetzt die Standard_small.xml als Standard in den Code gesetzt welche automatisch aufgerufen wird wenn kein Parameter übergeben wird.

    Das kam erst nach dem Refactoring. - bevor das Problem aufkam 😕

    0
  • E

    theSplit schrieb:

    In der Tat leaked der Code gerade ein paar Bytes

    Haha, bin ich wieder einmal drumrum gekommen einen Besen zu verspeisen. 😉

    0
  • T

    Wenn du mir nun auch sagst wo der Fehler liegt, darfst du auch auf dem Besen reiten 😉

    0
  • E

    theSplit schrieb:

    Wenn du mir nun auch sagst wo der Fehler liegt, darfst du auch auf dem Besen reiten 😉

    Siehe die erste Zeile von Wutz seiner Antwort.

    EDIT:
    Abgesehen davon bin ich schon auf so einigen Besen rumgeritten. 🙂

    EDIT #2:
    Dürfte höchstwahrscheinlich an ** liegen. Wieder ohne mir den ganzen code anzusehen. Aber mein Gefühl ist meistens ziemlich zuverlässig. (Ausser du hast was ganz grundsätzliches freizuugeben vergessen)

    0
  • T

    Wenn Wutz mir dann auch nochmal Feedback gibt was ich verbessern kann, mach ich das gern. 🙂

    @EOP
    Ich glaube ich komm in einen Hexenkessel wenn ich hier weitere herumstachele 😉

    0
  • E

    theSplit schrieb:

    Wenn Wutz mir dann auch nochmal Feedback gibt was ich verbessern kann, mach ich das gern. 🙂

    @EOP
    Ich glaube ich komm in einen Hexenkessel wenn ich hier weitere herumstachele 😉

    Nö, ich bin so ziemlich der einzige Schmutzfink hier. Die meisten Anderen sind "die Guten".

    0
  • Mod

    Es wäre gut, wenn du uns deine überarbeitete Version zeigen würdest. Und zwar sowohl vollständig (so dass man sie einfach kopieren und ausführen kann) als auch auf das wesentliche Problem reduziert. Siehe:
    Wie man Probleme nachstellbar und nachvollziehbar macht
    Am besten mit einer Beispielausgabe und einer Beschreibung, welche Ausgabe eigentlich erwartet wäre.

    Fast 1000 Zeilen Code wird sich niemand einfach mal eben so durchlesen. Zumal es bereits jede Menge Verdächtige gibt, da du, wie von Wutz und EOP (auf ihre etwas eigenartige Weise) beschrieben, ein paar schwere Programmiersünden begehst. Die Anzahl globaler Variablen sollte 0 sein, nicht 1 und erst recht nicht 3 und ob sie static sind oder nicht ist wurscht. Ebenso hast du offensichtlich Schwächen bei der manuellen Speicherverwaltung, die du anscheinend viel zu großmütig einsetzt und zudem scheinst du jede Menge Mythen aus schlechten Lehrbüchern aufgeschnappt zu haben, wie diese komische NULL-Prüfung vor dem free oder die vielen Casts. Zudem ist der Code äußerst unübersichtlich und es ist schwer Fehler da drin zu finden. Normalerweise sollte eine Funktion bloß ein paar Zeilen lang sein, du hast alles in einer einzigen Funktion!

    All dies, globale Variablen, (schlecht umgesetzte) manuelle Speicherverwaltung, Pointergefrickel und unübersichtlicher Code sind bekannt als sehr häufige Fehlerquellen. Es wäre ein Wunder, wenn es nicht da dran läge. Aber eine Verbesserung der unmittelbaren Ursache wäre ziemlich müßig. Solange die eigentlichen Ursachen, nämlich die genannten Programmiersünden, nicht behoben werden, wird ein Finden und Verbessern des unmittelbaren Fehlers bloß dazu führen, dass der nächste Fehler sich an anderer Stelle äußern wird. Eine Komplettüberarbeitung nach gängigen Best-Practices wäre daher wohl die beste Vorgehensweise. Höchstwahrscheinlich löst sich das Problem dabei von ganz alleine.

    0
  • T

    Ich versuche was ich machen kann. Ob ich alles nach "Best practice" umschreiben kann weiß ich allerdings nicht. Da fehlt mir auch das Know-How zu, da ich mich mit den Gewohnheiten von gestandenen C-Codern nicht auseinandergesetzt habe.

    So fern es Kritik gibt die man umsetzen kann, vielleicht auch wirklich nur sehr problemspezifisch, bin ich gerne Willens diese Umzusetzen.

    Aber das setzt auch voraus das man jemanden der kein Guru ist auch etwas mit der Nase auf das Problem stößt - außerdem, ich bin Hobby-Programmierer - so sieht der Code vermutlich auch aus, da ich selbst nicht einschätzen kann wo und wie man am besten die Kanten ausbügelt.

    Ist auch mein letztes Feedback für heute. Ich wollte nur mal darauf hinweisen das man nicht als Meister in einem Thema beginnt.

    Und ja, ich habe mit den Büchern von Jürgen Wolf angefangen die Grundlagen von C zu lernen. Darüber gibt es aber auch anderorts hier Threads wie man es besser nicht machen sollte, war aber zu spät.

    Trotzdem würde ich den Code ungern in das Nirvana schicken, weil ich die Routine zum lesen und schreiben von Tags schon einmal - mit meinem Wissensstand überarbeitet hatte. Daher wäre ich ja auch sehr offen für problemspezifische Kritik.

    Das hat aber auch nur wenig mit dem Grundproblem zu tun, es gibt ein paar Versionen vor dem aktuellen Stand wo keine Speicherleaks aufgetreten sind, aber der Code hals "dirty" runtergeschrieben war. Um auf eine Rückmeldung einzugehen.

    0
  • E

    Weil ich mir auch fast 1000 Zeilen code durchlese wenn ich nicht wirklich was für meine Interessen und für mich Nützliches lernen kann. Da bin ich durchaus eigenartig (und die meisten Anderen wahrscheinlich auch).

    0
  • T

    Ich kann ja versuchen das ganze auf ein Beispiel herunterzubrechen, aber es wird nicht diesen Umfang besitzen noch diese Komplexität wie es aktuell der Fall ist.

    Und das ist halt das geforderte Spektrum der Aufgabenstellung, ob ich das in Funktionen schreibe oder nicht.

    Was wollt ihr denn lernen? Nur Quality Code mit neuen Algorithmen und wissenschaftlichen Durchbrüchen? - Macht eurem Frust doch besser wo anders Platz als in diesem Thread, ich bin weder Wissenschaftler noch beruflicher Programmierer.

    Macht mich sauer diese Grundeinstellung gegenüber jemand neuem hier im Forum...

    Oder muß ich ganz vorsichtig schreiben "C Anfänger, erbitte um Problembeantwortung" ?

    Wenn ich wüßte wo der Fehler liegt würde ich Ihn selbst beantworten, dann kann ich von mir aus auch ein Beispiel dem zugrunde Aufbauen.

    0
  • Mod

    theSplit schrieb:

    Ich kann ja versuchen das ganze auf ein Beispiel herunterzubrechen, aber es wird nicht diesen Umfang besitzen noch diese Komplexität wie es aktuell der Fall ist.

    Das wäre das Beste und ist allgemein eine sehr gute Methode, einen Fehler zu finden. Niemand kann und will einen Fehler in 1000 Zeilen Code suchen, wenn 900 davon irrelevant sind. Wenn du beim Kürzen des Codes feststellst, dass der Fehler nicht mehr auftritt (dies ist nach jeder Kürzung zu prüfen!) dann liegt der Fehler höchstwahrscheinlich in dem gekürzten Teil und du wirst ihn sehr wahrscheinlich selber finden, wenn du dir anguckst, wieso der gekürzte Teil den Fehler verursacht. Deshalb habe ich dir den obigen Link gegeben, denn du bist schließlich nicht der erste, der mit einem extrem langen Programm ankommt und da drin einen bestimmten Fehler sucht. Es ist wichtig, zu lernen, wie man selber Fehler findet oder zumindest anderen die Fehlersuche erleichtert (denn das erleichtert sie einem auch selber).

    Und das ist halt das geforderte Spektrum der Aufgabenstellung, ob ich das in Funktionen schreibe oder nicht.

    Und? Du suchst doch eine Erklärung des Fehlers und nicht eine Lösung für die Aufgabenstellung, oder? Also reduzier den Code auf den Fehler!

    Was wollt ihr denn lernen? Nur Quality Code mit neuen Algorithmen und wissenschaftlichen Durchbrüchen? - Macht eurem Frust doch besser wo anders Platz als in diesem Thread, ich bin weder Wissenschaftler noch beruflicher Programmierer.

    Das sollte keine Ausrede sein, es nicht wenigstens zu versuchen. Wenn du sagst, dass du ruhig pfuschen darfst, weil du keine hohen Ansprüche an dich selber hast, ist es dann ein Wunder, wenn Pfusch heraus kommt? Es ist praktisch sicher, dass der Fehler in deinem Programmierstil begründet liegt. Wie sollen wir dir das sonst sagen, außer dass wir es dir eben sagen? Sollen wir so tun, als wäre das so in Ordnung? Das hilft dir doch auch nicht weiter.

    Macht mich sauer diese Grundeinstellung gegenüber jemand neuem hier im Forum...

    Verwechselte Direktheit nicht mit Grobheit! Wutz und EOP nehmen kein Blatt vor den Mund, wenn sie sagen, dass der Code eine Zumutung ist, dann weil er es ist. Wenn sie dich persönlich beleidigen wollten klänge das anders und jemand (das wäre wahrscheinlich ich) würde sie auch für solch eine Verfehlung zurecht weisen. Dies ist eine in Foren wie diese übliche Art der Kommunikation, wo es in erster Linie um das Lösen von Problemen geht und nicht darum dass der Fragesteller sich wohl oder geborgen fühlt.

    0
  • E

    Nicht mal ich hab bisher von SeppJ oder einem anderen mod oder admin eine Zurechtweisung einstecken müssen - und ich bin manchmal mehr als direkt.

    Es ärgert die Leute, die sich im Forum Mühe geben jemandem zu helfen wenn sie den Eindruck haben, daß du dir nicht selber Mühe gibst.

    Ist nichts persönliches.

    https://www.google.de/search?q=free+pointer+to+pointer+c

    0
  • T

    Okay, nach dieser Antwort bin ich auch wieder ein wenig besänftigt.

    Aber was ich dennoch nicht verstehe, ich kann von dem Parser wenig abstrahieren als ich schon, meines Wissenstandes nach, gemacht habe.

    Ich könnte ein paar mehr Comments setzen, das würde nicht den Code aufwerten, aber die Lesbarkeit erhöhen.

    Dennoch stehe ich vor diesem massigem switch/case / if else Fälle Konstrukt. Und da brauche ich halt Anleitungen was es besser zu machen gilt, das habe und kann ich mir nicht eben gerade weil es jemanden stört anlesen.

    Aber gut, ich arbeite an dem Code weiter und versuche auszulagern was geht, so sinnvoll es geht. Das mache ich aber auch nicht mehr heute. Es wird vielleicht mit dem richtigen Feedback kommen.

    Hier habe ich das leider bisher nicht gesehen, auch wenn ich verstehen kann das man nicht auf alles, aber zumindest auf einen Teil eingehen kann.

    Ich frage mich gerade warum ich ausfaktoriert habe, falsch meiner Annahme nach, um dann Fehler einzubringen, nur das sich niemand über die Main beschwert.

    Ein schweres Los.

    Ich arbeite weiter an dem Code und hoffe das ich Feedback bekomme um das zu Optimieren was alles beastandet hatten, bzw. ein Teil dessen.

    Dann kann ich mich auch um das kümmern was Sinn der Frage war, das Problem zu lösen.

    0
  • D

    Eine Sache will ich noch anmerken:
    Prüfung auf NULL vor free ergibt schon Sinn - aber nur, wenn man sich den Check in free sparen will und damit überhaupt den Overhead beim Aufruf. Der ist marginal - kann aber trotzdem relativ schnell aufsummieren. Ein Check ohne Aufruf geht meistens schneller als ein Aufruf und dann Check.

    Vernichtet aber so ein bisschen die Lesbarkeit, wenn man das überall durchsetzt. Ein kleines Makro kann hier helfen, sowohl Lesbarkeit zu erhöhen als auch den Overhead des Calls zu sparen.

    0
  • Mod

    Hier sieht mir das aber doch stark so aus, als könnten die Zeiger überhaupt nie NULL sein. Cargo Cult.

    0
  • D

    Wohl wahr. Aber einem Anfänger/Hobbyentwickler generell "Auf NULL prüfen vor free ist blöd" zu sagen ist doof, weil wir doch halt Gewohnheitstiere sind - auch bei schlechten Angewohnheiten. Und dann machen wir Sachen blind, ohne uns darüber zu wundern, warum wir sie so machen. Man sollte zumindest schon mal davon gehört haben - und irgendwann, wenn die Cycles dann zählen, erinnert man sich daran, dass es da diesen Sonderfall gab, mit dem man noch ein bisschen Geschwindigkeit herrauspressen kann ...

    Genauso verkettete Listen. Sind generell blöde, aber dann gibt es tatsächlich Anwendungsfälle, wo sie das Mittel der Wahl sind. Und wenn man vorher "verkettete Listen sind doof" sagt, dann raubt man einem Anfänger auch die Möglichkeiten, wo VLs halt die beste Performance liefern.

    War halt allgemein gesprochen, nicht unbedingt im direkten Bezug zum geposteten Code. Sondern nur als Notiz, damit der OP weißt, dass es Anwendungsfälle gibt, wo dies schon seinen Sinn ergibt.

    EDIT: "war" zu "wahr" - ich sollte nicht mehr völlig übermüdet Posts schreiben ...

    0

  • Auch dein geänderter Code ist noch viel zu groß, das hängt auch damit zusammen, dass deine Datenstruktur viel zu redundant ist.

    Statt 660 Zeilen tun es auch 70, dafür dann aber fehlerfrei:
    - keine globalen Variablen
    - für jeden Anwendungsfall eine Funktion
    - wenige Hilfsvariablen (steigert Übersichtlichkeit,...)
    - mit wenigen Handgriffen erweiterbar (z.B. zum HTML-Reader)
    - bei XML arbeitest du mit Tags, d.h. mit Strings und nicht mit Zeichen, deshalb ist deine zeichenweise Verarbeitung prinzipiell untauglich
    - struct-Zeiger-Listen sind unübersichtlich, einfache struct-Listen tun es auch
    - Rekursion ist die ultimative Wiederverwendung, minimiert Code, ist hier vertretbar (weil die Zahl der Rekursionsstufen absehbar unkritisch ist) aber dafür meist weniger verständlich
    ...
    ------------------------------
    - free fehlt noch
    - Fehlerbehandlung fehlt noch
    ...

    In 'print' musst du jetzt nur noch deinen JSON-Konverter einbauen, und fertig.

    /* XML-Minireader
   (c) Wutz@https://www.c-plusplus.net/forum/334128-20 */

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/stat.h> /* kein Standard, haben aber die meisten Compiler im Angebot */

typedef struct entry {
  char key[20];     /* einfache Arrays reichen hier erstmal */
  char value[1000];

  int i;
  struct entry *childs;
} Entry;

void add(Entry *e,    /* Wurzel-Element */
  const char *key,    /* Tagname der aktuellen Stufe */
  const char **s      /* Quasi-Stringstream als Input */
  )
{
  char t[3];
  memset(e, 0, sizeof*e);
  sprintf(e->key, "%.19s", key);
  while (1 == sscanf(*s, "%2s", t))
  {
    if (*t == '<'&&t[1] != '/')
    {
      char x[20];
      sscanf(strchr(*s, '<') + 1, "%19[^ >]", x);
      *s = strchr(*s, '>') + 1;
      e->childs = realloc(e->childs, ++e->i*sizeof*e);
      add(e->childs + e->i - 1, x, s);
    }
    else
    {
      if(!e->i)
        sscanf(*s, "%999[^<]", e->value);
      *s = strchr(*s, '>') + 1;
      return;
    }
  }
}

void print(const Entry *e, int n) /* eingerückte Ausgabe (3x space) für jede Stufe/Element */
{
  printf("%*s<%s>%s%s", n * 3, "", e->key, e->value,e->i?"\n":"");
  {
    int i;
    for (i = 0; i < e->i; ++i)
      print(e->childs+i, n + 1);
  }
  printf("%*s</%s>\n", (e->i?3:0)*n,"", e->key);
}

int main()
{
  Entry e;

  struct stat t;
  int x = stat("standard_small.xml", &t);
  char *s = calloc(1, t.st_size + 1);
  FILE *f = fopen("standard_small.xml", "rb");
  fread(s, t.st_size, 1, f);
  fclose(f);

  add(&e, "", &s);  /* Aufsammeln aus String und Baumstruktur füllen */
  print(&e, 0);     /* Baumelemente stufenweise ausgeben auf stdout  */

  return 0;
}

    <>
   <?xml>
      <site>
         <regions>
            <africa>
               <item>
                  <location>United States</location>
                  <quantity>1</quantity>
                  <name>duteous nine eighteen </name>
                  <payment>Creditcard</payment>
                  <description>
                     <parlist>
                        <listitem> officer embrace such fears distinction attires 
                           <text>
                                page rous lady idle authority capt professes stabs monster petition heave humbly removes rescue runs shady peace most piteous worser oak assembly holes patience but malice whoreson mirrors master tenants smocks yielded </text>
                        </listitem>
                     </parlist>
                  </description>
                  <listitem>
                     <text>
                                shepherd noble supposed dotage humble servilius bitch theirs venus dismal wounds gum merely raise red breaks earth god folds closet captain dying reek 
                            </text>
                  </listitem>
               </item>
            </africa>
            <shipping>Will ship internationally, See description for charges</shipping>
            <incategory>
               <incategory>
                  <incategory>
...
    0
  • T

    Ich danke dir für dein Codebeispiel und das du doch noch detailliertes Feedback abgegeben hast wie ich meinen Code verbessern könnte.

    Auch habe ich nicht gewusst das man mit Scanf in dieser Form arbeiten kann und quasi sogar bis zu Zeichengrenzen einlesen kann. Defintiv etwas das ich hätte mit verwenden können.

    Etwas Feedback zu deinem Code habe ich aber auch, so gut wie er geschrieben ist, das Resultat ist falsch bzw. machst du es dir zu einfach.

    - Die limitierte Datenlänge für Tag und Daten vorprogrammieren Datenverlust, warum nicht dynamisch?

    - es fehlt die Behandlung von Attributen: <book name="The Shogun" label="Red">
    Deine Lösung findet nur "book" und wirft die wichtigen Attribute komplett raus

    - Tags die keine Daten enthalten (im Datenbeispiel "<incategory>" Tags) werden einfach als Kind-Elemente angeordnet, obwohl sie auf gleicher Ebene liegen

    - Es fehlt ein Stripping der Whitespaces der Daten, es geht dadurch nichts verloren, aber wenn man die Daten zum Beispiel im Browser ausgibt (weil aus einer Datenbank kommend) hätte man ungewollte Textdarstellungen

    - eingebette Tags werden ignoriert und schlimmer Daten einfach abgeschnitten sobald ein Tag in einem Tag auftaucht

    und das kleinste Übel:
    - XML Deklaration wie das Encoding und XML Version werden mit kopiert

    Ich frage mich auch, wie deine Lösung es schaffen will Json Arrays zu bilden? Da fehlt jegliche Logik da du keine Informationen hast wie viele Elemente auf einer Ebene liegen und welche davon SubArrays beschreiben würden - das genannte Problem mit den Ebenen spielt dabei noch hinzu.

    Zu meiner Verteidigung:
    Meine Lösung ist zwar bei weitem nicht so professionell umgesetzt, aber sie verliert keine Daten und wertet mehr Informationen beim Parsen der Daten aus die später zum beschreiben der Json gebraucht werden würden.

    Also 70 Zeilen reichen noch nicht aus 😉

    Ich gebe aber zu, mir fehlt das Know-How wie ich richtig mit der Standardbibliothek(en) umgehe, zumal ich noch nicht alles verwendet habe.

    Noch kurz zum Thema Speicherleaks/Freeing:
    Diese sind behoben und die frees sind nun ohne Null Check.

    0

  • Deine Kritik ist reichlich naiv,
    - erstens steht im Kommentar, dass Arrays 'erstmal' reichen sollen
    - deine 'fehlenden' Attribute sind ohne Erhöhung der Codezeilen einfach aufnehmbar
    - Whitespace-Stripping ist nicht XML konform
    - der Code hat nur eine Stelle für potentielle Speicherlecks, im Gegensatz zu deinem
    - für die Weiterverarbeitung musst du natürlich den Gesamtpfad aufsammeln, aber genau das macht ja 'print'
    - du hast den Code nicht verstanden, du hast das Prinzip der Kapselung in Funktionen mit Übergabe des einen zentralen Objektes aus main nicht verstanden, du möchtest lieber >600 Zeilen pflegen statt 70, viel Spaß

    0
Anmelden zum Antworten