[C] inhoud .txt kopiëren naar dynamische structarray

Pagina: 1
Acties:

Acties:
  • 0 Henk 'm!

  • daboemann
  • Registratie: April 2007
  • Laatst online: 12:11
Hallo programmeurs,

Ik ben sinds kort van java overgestapt naar C, maar ben nu al de hele dag bezig om uit te zoeken waarom mijn heap corrupt raakt.

Even wat algemene informatie over mijn programma, ik moet een woordenboek console applicatie maken in C die een .txt file kan uitlezen en dit op slaat in een structarray die het volgende bevat:
C:
1
2
3
4
5
typedef struct _relation
{
    char word[30];
    char translation[30];
}relation;


De grootte van de array moet dynamisch zijn, dus wordt er in de main() een malloc aanroep gedaan om alvast wat geheugen te reserveren:
C:
1
2
3
4
 

relation *dictionary;
dictionary = (relation *) malloc(sizeof(relation));


Nu heb ik een functie geschreven die een woord toevoegt aan de dictionary, dit doet hij door eerst een realloc aanroep te doen en dan vervolgens de nieuwe plek te vullen met het nieuwe woord + vertaling:

C:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
/**
Voeg een element toe aan de dictionary array:
- realloc extra geheugen
- vul de gegevens in
- wordCount verhogen
**/
void addToDictionary(relation *dictionary, int **wordCount, char *word, char *translation){
    dictionary = realloc(dictionary, ((**wordCount + 1) * sizeof(struct _relation)));
    // de gegevens invullen vullen
    strcpy(dictionary[**wordCount].word, word); 
    strcpy(dictionary[**wordCount].translation, translation);

    //aantal woorden verhogen
    **wordCount += 1;
}


Dit werkt prima! Als ik hardcoded wat woorden wil toevoegen doet hij dit helemaal geweldig.

Nu wil ik een file uitlezen --> opslaan in een array --> info eruit verwerken (splitten op " ") --> de functie addToDictionary aanroepen.

Ik kan prima alles uitlezen wat in de file zit, splitten doet ie ook goed, maar naardat ik 2 woorden heb toegevoegd en dan aan de 3e begin, crasht het programma en geeft visual studio aan dat het mogelijk een corrupte heap is. Visuals studio opent dan dbgheap.c dus schijnbaar zit er iets met de heap verkeerd.. mijn vraag is wat? Ik zit nu al ruim 4 uur te kijken naar de code, stap voor stap debuggen en ik kom er nog niet uit.

De code voor het uitlezen + toevoegen aan het woordenboek:
C:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
/**
open een wordenboek bestand en lees hem uit   nu nog max 128 regels!
**/
void readDictionaryFile(relation *dictionary, int *wordCount, char *filename){

    FILE *file = fopen ( filename, "r" );
    int i, j;
    char content[128][128];
    char line[128]; 

    for(i=0; i<128; i++)
        for(j=0; j<128; j++) 
            content[i][j] = '\0';

    for(i=0; i<128; i++)
        line[i] = '\0';

    if ( file != NULL )
    { 
        i=0; 

        while ( fgets ( line, sizeof line, file ) != NULL ) /* read a line */
        {
            //fixInputEnd(line);
            strcpy(content[i], line);
            i++;
        }
        for(j = 0; j < i; j++){ 
            char *tmp = &content[j];
            char word[30],translation[30];
            printf("array ----> %s \n", &content[j]);       
            
            strcpy(word,strtok(tmp," "));
            strcpy(translation,strtok(NULL," "));
            addToDictionary(dictionary,&wordCount,word,translation);
        }

        fclose ( file );
        
    }
    else
    {
        perror ( filename ); /* why didn't the file open? */
    }

}

Acties:
  • 0 Henk 'm!

  • bobo1on1
  • Registratie: Juli 2001
  • Laatst online: 18-05 17:57
C:
1
&content[j]


Dat is geen char* maar een char**.
Daar zou de compiler ook warnings voor moeten geven.

[ Voor 28% gewijzigd door bobo1on1 op 11-10-2009 18:21 ]

Impedance, a measure of opposition to time-varying electric current in an electric circuit.
Not to be confused with impotence.


Acties:
  • 0 Henk 'm!

  • ValHallASW
  • Registratie: Februari 2003
  • Niet online
Voor het debuggen van geheugenfouten is valgrind altijd praktisch. Inhoudelijk ben ik op het moment even niet helder genoeg :P

Acties:
  • 0 Henk 'm!

  • daboemann
  • Registratie: April 2007
  • Laatst online: 12:11
bobo1on1 schreef op zondag 11 oktober 2009 @ 18:18:
C:
1
&content[j]


Dat is geen char* maar een char**.
Daar zou de compiler ook warnings voor moeten geven.
Klopt, dat is idd een onnodige & want het is al een array en dus een pointer. De compiler gaf inderdaad een warning die ik over het hoofd heb gezien (de rest van de warning zijn dat fopen en str*** niet veilig zijn en dat je eigenlijk de _s moet gebruiken). Ik heb het aangepast, maar mijn programma crasht nog steeds.

Ik zal die valgrind eens proberen. Dit is de eerste keer dat ik met malloc en realloc werk en dus memoryleak/corrupt problemen moet debuggen, ben benieuwd of hij iets aangeeft :) In ieder geval bedank voor de tip!

Acties:
  • 0 Henk 'm!

  • .oisyn
  • Registratie: September 2000
  • Laatst online: 17-09 14:05

.oisyn

Moderator Devschuur®

Demotivational Speaker

bobo1on1 schreef op zondag 11 oktober 2009 @ 18:18:
C:
1
&content[j]


Dat is geen char* maar een char**.
Daar zou de compiler ook warnings voor moeten geven.
Nee, dat is een char(*)[128], dat is niet eens converteerbaar naar een char**.

Give a man a game and he'll have fun for a day. Teach a man to make games and he'll never have fun again.


Acties:
  • 0 Henk 'm!

  • R4gnax
  • Registratie: Maart 2009
  • Laatst online: 06-09 17:51
daboemann schreef op zondag 11 oktober 2009 @ 17:08:
Hallo programmeurs,

Ik ben sinds kort van java overgestapt naar C, maar ben nu al de hele dag bezig om uit te zoeken waarom mijn heap corrupt raakt.

Even wat algemene informatie over mijn programma, ik moet een woordenboek console applicatie maken in C die een .txt file kan uitlezen en dit op slaat in een structarray die het volgende bevat:
C:
1
2
3
4
5
typedef struct _relation
{
    char word[30];
    char translation[30];
}relation;


De grootte van de array moet dynamisch zijn, dus wordt er in de main() een malloc aanroep gedaan om alvast wat geheugen te reserveren:
C:
1
2
3
 
relation *dictionary;
dictionary = (relation *) malloc(sizeof(relation));
Niet of het één of ander, maar dit is natuurlijk bagger wanneer het op efficientie aankomt. Je wilt tenminste in batches elementen bij alloceren met realloc, aangezien dat een dure operatie is. Je kunt dan bijhouden welke elementen nog niet in gebruik zijn en telkens wanneer er een nieuw woord aan je dictionary toegevoegd wordt, zo'n 'leeg' relation structje in gebruik nemen. Zijn de lege elementen op, dan is het tijd om weer een nieuwe batch te alloceren.

Verder zou ik de file parsing routine herschrijven. Let eens goed op je relation struct: je kunt nooit woorden opslaan die langer dan 30 characters zijn. Daarbij is het 30e character zelfs altijd een null terminator, tenzij je het volle gebruik van de 30 characters in een word als speciale case afhandelt in de code die met deze dictionary moet gaan werken. (En dat zou ik je niet aanraden...)

Dus je file reading loop wordt dan zoiets als:
  1. Lees 30 characters naar een char[30] buffer.
  2. Scan deze 30 characters voor de eerste spatie, line break of '\0' character.
  3. Verander indien het een spatie of line break betreft, dit character in een '\0' character.
  4. Kopieer de buffer naar je dictionary met je daarvoor geschreven functie. (strncpy kopieert t/m de eerste null terminator, geloof ik.)
  5. Memcpy alles achter de null terminator naar de start v/d buffer.
  6. Lees voldoende extra characters uit het file om de buffer weer verder op te vullen, mits het file nog zoveel characters bevat. Bevat het file zoveel characters niet meer, lees er dan zoveel mogelijk en pad de buffer met '\0'.
  7. Indien de buffer nog woorden bevat: ga naar stap 2.
Je maakt het jezelf nodeloos complex door hier intern met een 2 dimensionaal array de input lines van het txt file te emuleren...

Acties:
  • 0 Henk 'm!

  • daboemann
  • Registratie: April 2007
  • Laatst online: 12:11
.oisyn schreef op zondag 11 oktober 2009 @ 20:09:
[...]

Nee, dat is een char(*)\[128], dat is niet eens converteerbaar naar een char**.
Hmm, valgrind gaat hem toch niet worden, want zit onder windows :P

Ik blijf het raar vinden dat het twee keer helemaal perfect gaat en dan pas vanaf de 3e ineens het programma crasht.


@ R4gnax

Bedankt voor je reactie.

Dat die realloc bij elke toevoeging van woorden gebeurt, is inderdaad niet efficient en het zou inderdaad beter zijn om bijvoorbeeld 10 of 100 plaatsen te reserveren. Daarvoor moet ik wel het een en het ander omgooien (var erbij die bijhoudt hoeveel plaatsen er beschikbaar zijn) en dat zal ik eens gaan doen.

Dat ik maar maximaal 29 letterwoorden op kan slaan is de bedoeling, grotere woorden hoef ik niet op te kunnen slaan.

Woordenboek.txt bevat bijvoorbeeld dit:
hoi hi
auto car
eten food
enz..

Ik volg de gedachtegang achter jou manier, maar ik moet om dingen in mijn dictionary te zetten zowel het woord als de vertaling hebben (om de functie addToDictionary aan te kunnen roepen). Dus als ik een buffer van 30 chars uitlees en dan kijk of dat genoeg is--> zo niet lees ik een tweede buffer uit, dan kan ik toch niet na die eerste buffer uitlezen al een woord toevoegen aan mijn dictionary? Want dan zou ik de vertaling niet hebben.


edit, heb mn code omgebouwd:

C:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
void addToDictionary(relation *dictionary, int **wordCount, char *word, char *translation){
    if((**wordCount + 1)% 10 == 0){ //er zijn weer 10 woorden erin gezet--> realloc 10 extra dingen
        dictionary = realloc(dictionary, ((**wordCount + 10) * sizeof(struct _relation)));
    }
    // de relatie vullen
    strcpy(dictionary[**wordCount].word, word); 
    strcpy(dictionary[**wordCount].translation, translation);

    //aantal woorden verhogen
    **wordCount += 1;   
}

/**
open een wordenboek bestand en lees hem uit
**/
void readDictionaryFile(relation *dictionary, int *wordCount, char *filename){

    FILE *file = fopen ( filename, "r" );
    int i;
    char line[128]; 

    for(i=0; i<128; i++)
        line[i] = '\0';

    if ( file != NULL )
    { 
        while ( fgets ( line, sizeof line, file ) != NULL ) /* read a line */
        {
            char word[30],translation[30];
            fixInputEnd(line);          
            //printf("array ----> %s \n", &content[j]);
            strcpy(word,strtok(line," "));
            strcpy(translation,strtok(NULL," "));
            addToDictionary(dictionary,&wordCount,word,translation);
            for(i=0; i<128; i++)
                line[i] = '\0';

        }
        fclose ( file );
        
    }
    else
    {
        perror ( filename ); /* why didn't the file open? */
    }
}



de output is nu voordat ie crasht: (ik doe een print van de hele dictionary na elke toevoeging)
1: "eten"-->"food"
2: "hoi"-->"hi"
3: "en"-->"and"
4: "of"-->"or"
5: "hallo"-->"hello"
6: "eten"-->"food"
7: "hoi"-->"hi"
8: "en"-->"and"
9: "════════════════════════════════════════════════════════════hallo"-->"══════
════════════════════════hallo"
10: "hallo"-->"hello"
11: "eten"-->"food"
12: "hoi"-->"hi"
13: "en"-->"and"
14: "of"-->"or"
15: "hallo"-->"en"
16: "eten"-->"food"
17: "hoi"-->"hi"
18: "en"-->"and"
Hij doet dus iets verkeerd bij de eerste realloc.

Als hij bij regel 10 in de txt file komt, waar 'of or' staat, wordt de realloc aangeroepen. Vervolgens wordt er gewoon goed op het 9e plekje (tel plek 0 ook mee) de waarde opgeslagen.

Als hij dan bij regel 11 komt waar 'hallo hello' staat, zet hij deze neer op het 10e plekje, maar ben ik om de een of andere reden mijn 9e plekje kwijt, want sinds het 10e plekje gevuld is met 'hallo hello' staat er op plek 9 "════════════════════════════════════════════════════════════hallo"-->"══════
════════════════════════hallo"


Vervolgens knalt het programma eruit bij regel 19, maar het gaat dus al eerder mis :?

[ Voor 112% gewijzigd door daboemann op 11-10-2009 21:51 ]


Acties:
  • 0 Henk 'm!

  • R4gnax
  • Registratie: Maart 2009
  • Laatst online: 06-09 17:51
C:
1
if((**wordCount + 1)% 10 == 0){


Dat klopt niet.

Er van uitgaande dat wordCount het aantal woorden in de dictionary bijhoudt, komt wordCount overeen met de index waar het volgende element in opgeslagen gaat worden. Als je 10 elementen alloceert, dan heb je element 0-9 en is wordCount 10. Bij de eerstvolgende aanroep moet dan geheralloceerd worden.

Correcte oplossing zou dus moeten zijn:
C:
1
if(!(**wordCount % 10)) {



offtopic:
Goh, ik weet weer helemaal waarom ik zo blij was met C++ en STL...

[ Voor 9% gewijzigd door R4gnax op 11-10-2009 22:25 ]


Acties:
  • 0 Henk 'm!

  • R4gnax
  • Registratie: Maart 2009
  • Laatst online: 06-09 17:51
daboemann schreef op zondag 11 oktober 2009 @ 20:55:
Ik volg de gedachtegang achter jou manier, maar ik moet om dingen in mijn dictionary te zetten zowel het woord als de vertaling hebben (om de functie addToDictionary aan te kunnen roepen). Dus als ik een buffer van 30 chars uitlees en dan kijk of dat genoeg is--> zo niet lees ik een tweede buffer uit, dan kan ik toch niet na die eerste buffer uitlezen al een woord toevoegen aan mijn dictionary? Want dan zou ik de vertaling niet hebben.
Wat dacht je van zoiets?

C:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
#define WORDSIZE 30

void readDictionaryFile(relation* dictionary, int* wordCount, char* fileName)
{
  FILE* file;
  char  bufferA[WORDSIZE + 1]; // +1: always have a null terminator
  char  bufferB[WORDSIZE + 1];

  char  word[WORDSIZE];
  char  translation[WORDSIZE];

  char* buffer;
  char* offset;
  char* dest;

  int   remains;

  memset(bufferA, 0, WORDSIZE * sizeof(char));
  memset(bufferB, 0, WORDSIZE * sizeof(char));

  file = fopen(fileName, "r");
  if (file == NULL) perror ("Error opening file");
  else
  {
    buffer = bufferA;
    offset = buffer;
    dest = word;

    while(1)
    {
      // Pull enough characters to fill the regular part of the buffer
      offset = fgets(offset, WORDSIZE - (offset - buffer), file);
      if (offset == NULL && *buffer == 0) break;
    
      offset = strchr(buffer, ' ');
      if (offset != NULL)
      {
        *offset = 0;
      }
      else
      {
        offset = strchr(buffer, 0);
      }

      // Copy word and swap input destination
      strncpy(dest, buffer, WORDSIZE);
      dest = (dest == word) ? translation : word;
      
      // Swap and align buffers. Note that offset+1 is always valid as
      // buffer is WORDSIZE+1 in size! The spare null terminator
      // assures the remainder of the old buffer is copied to the start
      // of the new buffer, while strncpy pads the remainder of the new
      // buffer with null terminators. We reset the offset to where the
      // next fgets call should append characters
      remains = WORDSIZE + (buffer - offset);
      buffer = (buffer == bufferA) ? bufferB : bufferA;
      strncpy(buffer, offset+1, WORDSIZE+1);
      offset = buffer + remains;     

      // A pair of word and translation is ready
      if (dest == word)
      {
        addToDictionary(dictionary, wordcount, word, translation);
      }
    }
  

    fclose(file);
  }
}


Off-by-one pointer fouten usw. voorbehouden, uiteraard. Ik doe dit tenslotte even 'off the top of my head'. Je zult in ieder geval de redenering wel kunnen volgen, denk ik. ;)


offtopic:
Oops. dubbel gepost. Nou ja; ik zie geen delete knopje, dus laat dan maar zo.

Acties:
  • 0 Henk 'm!

  • daboemann
  • Registratie: April 2007
  • Laatst online: 12:11
R4gnax schreef op zondag 11 oktober 2009 @ 22:24:
C:
1
if((**wordCount + 1)% 10 == 0){


Dat klopt niet.

Er van uitgaande dat wordCount het aantal woorden in de dictionary bijhoudt, komt wordCount overeen met de index waar het volgende element in opgeslagen gaat worden. Als je 10 elementen alloceert, dan heb je element 0-9 en is wordCount 10. Bij de eerstvolgende aanroep moet dan geheralloceerd worden.

Correcte oplossing zou dus moeten zijn:
C:
1
if(!(**wordCount % 10)) {



offtopic:
Goh, ik weet weer helemaal waarom ik zo blij was met C++ en STL...
Die + 1 mocht inderdaad weg en dat verhelpt de fout op plaats 9 in de array:). Maar nu knalt mijn programma er na 16 woorden toe gevoegd te hebben uit.
Unhandled exception at 0x1026f693 (msvcr90d.dll) in Woordenboek_in_C.exe: 0xC0000005: Access violation reading location 0x00000000.
Waar hij er precies uit knalt is iets dat ik morgen ga uitzoeken met nog een keer stap voor stap debuggen.

Leuk zo'n C programmatje als je JAVA gewend bent dat vrij lastig is om te crashen :P


EDIT:
zie nu dat je ook wat code geschreven hebt, bedankt! Ga er morgen even op mijn gemak naar kijken, want als ik dat nu doe slaap ik ddl met dansende 1'en en 0'en voor mijn ogen

[ Voor 7% gewijzigd door daboemann op 11-10-2009 22:47 ]


Acties:
  • 0 Henk 'm!

  • leuk_he
  • Registratie: Augustus 2000
  • Laatst online: 15-07 15:35

leuk_he

1. Controleer de kabel!

Je geeft een pointer mee naar een lijst, realloc wijzigd die pointer, maar die pointer is als automatic ipv reference variable meegegeven.


code:
1
2
3
void addToDictionary(relation **dictionary, int **wordCount, char *word, char *translation){
    *dictionary = realloc
..



maar vooral
addToDictionary(&dictionary,&wordCount,word,translation);
zodat de gewijzigde waarde die uit realloc komt doorgaat naar het hoofdprogramma.


(Of zeg ik nou iets doms, want ik kan me bijna niet voorstellen dat al de mensen boven me dat niet meteen zien) :N :Y

[ Voor 17% gewijzigd door leuk_he op 11-10-2009 23:49 ]

Need more data. We want your specs. Ik ben ook maar dom. anders: forum, ff reggen, ff topic maken
En als je een oplossing hebt gevonden laat het ook ujb ff in dit topic horen.


Acties:
  • 0 Henk 'm!

  • bobo1on1
  • Registratie: Juli 2001
  • Laatst online: 18-05 17:57
.oisyn schreef op zondag 11 oktober 2009 @ 20:09:
[...]

Nee, dat is een char(*)\[128], dat is niet eens converteerbaar naar een char**.
Dat is prima te converteren, alleen heb je dan een pointer waar je verder niet veel mee kunt.

[ Voor 12% gewijzigd door bobo1on1 op 12-10-2009 00:56 ]

Impedance, a measure of opposition to time-varying electric current in an electric circuit.
Not to be confused with impotence.


Acties:
  • 0 Henk 'm!

  • djexplo
  • Registratie: Oktober 2000
  • Laatst online: 07-07 15:40
code:
1
dictionary = (relation *) malloc(sizeof(relation));

Dit mag je niet zo doen ... include "stdlib.h" dan is die cast niet nodig, en krijg je netjes een Null pointer terug als het niet lukt om geheugen te reserveren.

'if it looks like a duck, walks like a duck and quacks like a duck it's probably a duck'


Acties:
  • 0 Henk 'm!

  • .oisyn
  • Registratie: September 2000
  • Laatst online: 17-09 14:05

.oisyn

Moderator Devschuur®

Demotivational Speaker

bobo1on1 schreef op maandag 12 oktober 2009 @ 00:06:
[...]

Dat is prima te converteren, alleen heb je dan een pointer waar je verder niet veel mee kunt.
Ik had het over een impliciete conversie, en niet zoals een int* naar een float* te "converteren" is. Mijn punt was dat een char** en een char(*)[128] compleet ongerelateerd zijn. De ene point naar een pointer, de ander point naar een array. En &content[i] geeft je een pointer naar een array (die niet als pointer naar pointer te gebruiken is)
djexplo schreef op maandag 12 oktober 2009 @ 00:18:
code:
1
dictionary = (relation *) malloc(sizeof(relation));

Dit mag je niet zo doen ...
Wat is dat nou voor onzin, er is niets mis met die aanroep. Goed, de cast is overbodig, je kunt in C een void* impliciet naar T* casten, maar dat maakt het niet meteen fout. Overigens blijkt nergens uit of hij nou wel of niet (al dan niet indirect) stdlib.h includet

[ Voor 69% gewijzigd door .oisyn op 12-10-2009 11:30 ]

Give a man a game and he'll have fun for a day. Teach a man to make games and he'll never have fun again.


Acties:
  • 0 Henk 'm!

  • daboemann
  • Registratie: April 2007
  • Laatst online: 12:11
Je geeft een pointer mee naar een lijst, realloc wijzigd die pointer, maar die pointer is als automatic ipv reference variable meegegeven.

maar vooral addToDictionary(&dictionary,&wordCount,word,translation);
zodat de gewijzigde waarde die uit realloc komt doorgaat naar het hoofdprogramma.
_/-\o_ ik ben net voor ik het heb veranderd eens gaan debuggen met dat in het achterhoofd en inderdaad hij veranderd het adres! :o Ik weet niet waarom zij (als het niet werkt is het vrouwelijk, les 1 informatica die ik heb gehad jaren terug) het dan nodig vond om halverwegen, ergens bij regel 16 te crashen ipv gelijk nadat hij voor het eerst een realloc aanroept.


Maar het is dus zo: realloc kan de pointer wijzigen (omdat er niet genoeg extra geheugen beschikbaar is op de plek waar hij eerst naar toe aan het pointen was), die gewijzigde pointer sloeg ik niet op --> mijn programma veranderd intern wel de geheugenplek waar de struct array in werd opgeslagen, mijn pointer wijst nog steeds naar de oude plek --> knalt eruit omdat mijn pointer verwijst naar een ongebruikt stuk geheugen (het is immers verplaatst)

Ik krijg nu nog wel een warning die op meerdere plekken terugkomt (en toch werkt alles :P)
' relation *' differs in levels of indirection from 'relation **' bij de aanroep van de functie (zie hieronder)
en different types for formal and actual parameter 1
C:
1
2
3
4
5
6
//header
void addToDictionary(relation **dictionary, int **wordCount, char *word, char *translation);
                                    ^                           
                     komt niet overeen met
//aanroep  (in main)        v
readDictionaryFile(&dictionary,&wordCount,"c:\\woordenboek.txt");

Acties:
  • 0 Henk 'm!

  • .oisyn
  • Registratie: September 2000
  • Laatst online: 17-09 14:05

.oisyn

Moderator Devschuur®

Demotivational Speaker

Je copypaste de verkeerde code denk ik, want readDirectionaryFile() is een andere functie dan addToDictionary() ;). Dat het wel werkt vind ik vreemd, tenzij je op andere plekken die verkeerde indirectie weer teniet doet.

[ Voor 31% gewijzigd door .oisyn op 12-10-2009 11:31 ]

Give a man a game and he'll have fun for a day. Teach a man to make games and he'll never have fun again.


Acties:
  • 0 Henk 'm!

  • sam.vimes
  • Registratie: Januari 2007
  • Laatst online: 08-06 08:44
daboemann schreef op zondag 11 oktober 2009 @ 17:08:
C:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
typedef struct _relation
{
    char word[30];
    char translation[30];
}relation;

relation *dictionary;
dictionary = (relation *) malloc(sizeof(relation));

/**
Voeg een element toe aan de dictionary array:
- realloc extra geheugen
- vul de gegevens in
- wordCount verhogen
**/
void addToDictionary(relation *dictionary, int **wordCount, char *word, char *translation){
    dictionary = realloc(dictionary, ((**wordCount + 1) * sizeof(struct _relation)));
    // de gegevens invullen vullen
    strcpy(dictionary[**wordCount].word, word); 
    strcpy(dictionary[**wordCount].translation, translation);

    //aantal woorden verhogen
    **wordCount += 1;
}

/**
open een wordenboek bestand en lees hem uit   nu nog max 128 regels!
**/
void readDictionaryFile(relation *dictionary, int *wordCount, char *filename){

    FILE *file = fopen ( filename, "r" );
    int i, j;
    char content[128][128];
    char line[128]; 

    for(i=0; i<128; i++)
        for(j=0; j<128; j++) 
            content[i][j] = '\0';

    for(i=0; i<128; i++)
        line[i] = '\0';

    if ( file != NULL )
    { 
        i=0; 

        while ( fgets ( line, sizeof line, file ) != NULL ) /* read a line */
        {
            //fixInputEnd(line);
            strcpy(content[i], line);
            i++;
        }
        for(j = 0; j < i; j++){ 
            char *tmp = &content[j];
            char word[30],translation[30];
            printf("array ----> %s \n", &content[j]);       
            
            strcpy(word,strtok(tmp," "));
            strcpy(translation,strtok(NULL," "));
            addToDictionary(dictionary,&wordCount,word,translation);
        }

        fclose ( file );
        
    }
    else
    {
        perror ( filename ); /* why didn't the file open? */
    }

}
Een aantal zaken is hierboven al genoemd, maar:
  • Ik mis allerlei foutafhandeling (returncodes van malloc(), realloc(), fopen(), etc.)
  • Controle op maximale lengte van ingelezen word en translation (moet voor regel 58).
  • line 17 kan de pointer dictionary veranderen, maar die waarde wordt vergeten bij return uit addToDictionary(), zodat er een memory leak/dangling pointer ontstaat.
    Je moet de definitie van addToDictionary() daarom veranderen in:
    C:
    1
    2
    3
    4
    
    void addToDictionary(relation **pDictionary,...)
    {
        *pDictionary = realloc(*pDictionary,...);
        /* hier natuurlijk controle op *pDictionary == NULL */
  • de dubbele indirectie naar **wordCount in addToDictionary() is onnodig. In readDictionary() heb je al het adres van wordCount.
    Je moet de definitie van addToDictionary() daarom veranderen in:
    code:
    1
    
    void addToDictionary(..., int *pWordCount, ...) {

    (Ik geef de voorkeur aan een p-prefix om aan te geven dat het een pointer naar een returnwaarde betreft.)
    Gebruik pWordCount daarna als volgt:
    code:
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    
        int wc;
        relation *entry;
    
        ++*pWordCount; /* we voegen een entry toe: ophogen aantal */
        wc = *pWordCount; /* voor 't gemak */
        /* allocatie van ruimte voor wc entry's, 0 .. wc-1 */
        *pDictionary = realloc(*pDictionary, wc * sizeof (*pDictionary)[0]));
        entry = (*pDictionary)+wc-1;
    
        /* zorg ervoor dat word en translation maximaal 29 characters lang zijn! */
        strcpy(entry->word, word);
        strcpy(entry->translation, translation);
  • het is zeer aan te raden om strings en structs die in een functie ongewijzigd blijven, als pointer naar const te declareren. Wijzig daarom addToDictionary() in:
    C:
    1
    
    void addToDictionary(..., const char *word, const char *translation){


    De uiteindelijke declaratie van addToDictionary wordt dus:
    C:
    1
    
    void addToDictionary(relation **pDictionary, int *pWordCount, const char *word, const char *translation) {

    en de aanroep uit readDictionaryFile():
    C:
    1
    
    addToDictionary(&dictionary, wordCount, word, translation);
  • in readDictionaryFile() geldt hetzelfde voor dictionary en char * als in addToDictionary():
    code:
    1
    2
    3
    4
    
    void readDictionaryFile(relation **dictionary, int *wordCount, const char *filename) {
        /* dubbele indirectie voor **dictionary omdat *dictionary kan wijzigen */
        /* const char * filename omdat filename niet wijzigt (in principe) */
    }
  • iets ruimhartiger commentaar in de code mag wel.
  • vullen met nullen (lines 36-41) kan wel wat efficiënter
    code:
    1
    2
    
    memset(content, 0, sizeof content);
    memset(line, 0, sizeof line);

    hoewel ik betwijfel of dat sowieso wel nut heeft.
  • het is efficiënter om readDictionaryFile() de ingelezen regels direct te verwerken, in plaats van eerst alles inlezen en daarna alles verwerken. In dat geval kan de array content[][] verdwijnen en zit je ook niet meer vast aan een maximum van 128 woorden & hun vertaling.
    C:
    1
    2
    3
    4
    5
    6
    7
    8
    
    *pDictionary = NULL;
    *pWordCount = 0;
    while (fgets(line, sizeof line, file) != NULL) /* read a line */
    {
        word = strtok(line, " ");
        translation = strtok(NULL, " ");
        addToDictionary(pDictionary, pWordCount, word, translation);
    }

[ Voor 2% gewijzigd door sam.vimes op 13-10-2009 12:59 . Reden: twee regels verwisseld ]

Pagina: 1