Printing struct field prints only the last element multiple times - C

Question

I have a struct:

typedef struct fileRep
{
    char *fileName;

    int index;
    int visited;
} fileRep;

My main function has this call to a function that creates an array of struct filRep:

fileRep  *allFiles = (fileRep*)malloc(MAX_LINES*sizeof(fileRep));
int i = linesToStruct(allFiles, fp);

This is the function:

int linesToStruct(fileRep* allFiles, FILE* fp)
{
char line[MAX_CHARS_LINE];
int i = 0;
while(fgets(line, sizeof line, fp))
{

    fileRep file1 = createBasicFileStruct(line, i);
    allFiles[i] = file1;
    printf("%s is added\n", allFiles[i]);
    i++;
}
return i;
}

which calls this function:

fileRep createBasicFileStruct(char *line, int lineNumber)
{
char* name;
fileRep file1;
name = strtok(line, ":" );
file1.fileName = name;
file1.index = lineNumber;
file1.visited = 0;
return file1;
}

Now i try to iterate and print the fileName field of each fileRep struct i created:

int k = 0;
for(; k < i; k++)
{
    printf("File %d is %s\n", k, allFiles[k].fileName);
}

This results in this printing:

File 0 is file4
File 1 is file4
File 2 is file4
File 3 is file4

Now just some more info: The function lineToStruct has an internal test print, which prints:

file1.h is added
file2.h is added
file3.h is added
file4.h is added

Meaning it does add the files.

I am new to C and i am having trouble getting the pointer and array correlation, Why allFiles[0] is NULL? How do i set it to iterate properly?

_Now Let's assume that the function linesToStruct files allFiles nicely. it returns i which is the number of elements it added._ We can not assume that without seeing the code ;) — David Ranieri, Aug 13 '16 at 11:08
Great, and now please show us the `createBasicFileStruct` function LOL ;) where are you assigning the string to the member `filename`? — David Ranieri, Aug 13 '16 at 11:18
Shouldn't this `allFiles[i].fileName)` be `allFiles[k].fileName)`? — alk, Aug 13 '16 at 11:21
Aaah, the common `strtok())` trap, which I already smelled when reading "*... prints **only the last** element ....*" :-) — alk, Aug 13 '16 at 11:23
alk - thanks, this was a typo but it did not solve it, it now prints the last element: — Eyzuky, Aug 13 '16 at 11:23
is it that strtok moved my pointer? how do i take it back :) — Eyzuky, Aug 13 '16 at 11:23
`strtok` returns a ***pointer*** (in)to `line`. It's the ***same*** memory on ***each*** iteration, namely the memory of 'line`. — alk, Aug 13 '16 at 11:26
Alright then, why is it related to my problem? line is just the line i am reading from a text file argument, i am parsing it to the name and create a struct regardless with that name. — Eyzuky, Aug 13 '16 at 11:28
Learn to use a debugger to trace the code while inspecting all variables to learn what is really going on. — alk, Aug 13 '16 at 11:28
"*line is just the line i am reading from a text file*", no it isn't, it is a variable, which gets "*filled*" by what was read. It's the same variable on each iteration. — alk, Aug 13 '16 at 11:29
Think twice: What exactly does the code store in `file1.fileName`? — alk, Aug 13 '16 at 11:32
Answer this question: "*What exactly does the code store in `file1.fileName`?*" and become enlightened. ;-) — alk, Aug 13 '16 at 11:36
Actually, when adding this print message after file1.fileName = name; i get that each iteration prints a different file like i wanted it to.. so i think that is fine — Eyzuky, Aug 13 '16 at 11:40
I would really love it if you enlight me with an answer, i just don't get it. If i have to guess, it prints the files fileName of file1 just given by function createBasicFileStruct which returns a new fileRep struct. By the way this was solved by changing line to strdup(line) — Eyzuky, Aug 13 '16 at 11:44

David Ranieri · Accepted Answer · 2016-08-13T12:09:25.140

1

The contents of line are replaced on each iteration of fgets in the function linesToStruct, you can pass a duplicate of the string (you can use strdup(line) for this) instead of line.

Or better yet (as suggested by @alk), pass line and then duplicate the tokenized string inside createBasicFileStruct if you don't need the rest of tokens:

name = strtok(line, ":" );
file1.fileName = strdup(name);

In this way you need less space.

Don't forget to free all those strings at the end.

edited Aug 13 '16 at 12:09

answered Aug 13 '16 at 11:27

David Ranieri

39,972
7
52
94

I don't see the memory leak. I don't even see any `[m/c/re]alloc()`, in fact. – EOF Aug 13 '16 at 11:28
Notice my print inside function linesToStruct, it does show that it gets a new name every iteration.. – Eyzuky Aug 13 '16 at 11:31
@EF: `fileRep *allFiles = (fileRep*)malloc(MAX_LINES*sizeof(fileRep));` – David Ranieri Aug 13 '16 at 11:32
It is not all of the code, there is a free(allFiles); line – Eyzuky Aug 13 '16 at 11:33
@AlterMann: Uh, if `allFiles` is allocated by `malloc()`, modifying `allFiles[n]` is perfectly OK, and not a memory leak in any way, shape or form. – EOF Aug 13 '16 at 11:34
@EOF: oops, you are right, they are pointers not elements, I didn't see the `typedef`. Edited thanks ;) – David Ranieri Aug 13 '16 at 11:35
Like this? fileRep file1 = createBasicFileStruct(strdup(line), i); – Eyzuky Aug 13 '16 at 11:37
@Eyzuky: Yes, try it. – David Ranieri Aug 13 '16 at 11:39
Duplicating the buffer to be tokenised instead of the tokenisation's result is a ...well ... unconventional way to solve this. – alk Aug 13 '16 at 11:40
Wow alter mann that actually did the trick!!! – Eyzuky Aug 13 '16 at 11:42
@alk, `free` doesn't care about multiple `NUL` (`\0`) terminations in the string, isn't it? – David Ranieri Aug 13 '16 at 11:42
@Eyzuky, I know hahaha, don't forget to free the `filename` member when no longer needed. – David Ranieri Aug 13 '16 at 11:43
@AlterMann , a little noobish question - why do i need to free it if i did not call malloc for it? and also, why won't it be needed? I will need it to be a field in my struct for later in my code – Eyzuky Aug 13 '16 at 11:46
@Eyzuky, `strdup` reserves space using `malloc`: _The strdup() function shall return a pointer to a new string, which is a duplicate of the string pointed to by s1. The returned pointer can be passed to free()._ – David Ranieri Aug 13 '16 at 11:48
1

@AlterMann thanks, you the man (of alter) – Eyzuky Aug 13 '16 at 11:52
So is it line or fileName that i should free? because i am giving line to strdup – Eyzuky Aug 13 '16 at 11:56
You need to free the member `filename` of each element of the array (maybe in a loop) when you no longer need those strings (not before). – David Ranieri Aug 13 '16 at 11:58
1

@Eyzuky, you have an edit – David Ranieri Aug 13 '16 at 12:03

score 0 · Answer 2 · answered Aug 13 '16 at 11:25

0

Here:

int k = 0;
for(; k < i; k++)
{
    printf("File %d is %s\n", k, allFiles[i].fileName);
}

Shouldn't this be:

int k = 0;
for(; k < i; k++)
{
    printf("File %d is %s\n", k, allFiles[k].fileName);
}

??

answered Aug 13 '16 at 11:25

Nunchy

948
5
11

Thanks, i already edited the question, it was a typo but it also does not solve the problem, it still prints the last file only multiple times – Eyzuky Aug 13 '16 at 11:26

Printing struct field prints only the last element multiple times - C

2 Answers2