I have a task to read words from a file and print each word separately. So far I got this:

#define MAX 100000
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
typedef struct node *ptr;
typedef struct node {
    char *data;
    ptr next;
} node;

ptr head = NULL;

void add(char *data) {
        node *new_node = malloc(sizeof(node));
        new_node->data = data;
        new_node->next = head;
        head = new_node;
    printf("Adding new word:%s\n",data);
}

void print_list() {
    node *current = head;
        while (current != NULL) {
            printf("%s\n", current->data);
            current = current->next;
        }
}

void free_list(ptr *hnode)
{
    ptr p;
    while(*hnode){
        p = *hnode;
        *hnode = (*hnode)->next;
        free(p);
    }
}

int main(int argc, char *argv[]) {
    FILE *file;
    char buffer[MAX];
    char ch;
    int i = 0;

    if (argc != 2) {
        printf("Usage: %s <file>\n", argv[0]);
            exit(EXIT_FAILURE);
        }

        file = fopen(argv[1], "r");
        if (file == NULL) {
            printf("Error opening file\n");
            exit(EXIT_FAILURE);
        }

    while(i < MAX-1 && !feof(file))
    {
        if((ch = fgetc(file)) == ' '|| ch == '\n')
        {
            buffer[i] = '\0';
            add(buffer);
            i = 0;
        }
        else{
            if(ch == EOF)
                  break;
            buffer[i  ] = ch;
        }
    }

        print_list();
        free_list(&head);
        exit(EXIT_SUCCESS);
}

When I try to run the code there are no compilations or runtime errors but I get only the last word and a weird symbol, for example:

Adding new word:Hello
Adding new word:World!
Adding new word:Does
Adding new word:my
Adding new word:code
Adding new word:work?
work?
work?
work?
work?
work?
work?

The part when it says adding new word is correct and reads the file properly, my problem is the printing part. Edit: Now I have a proper EOF check, still doesn't work as intended.

CodePudding user response：

When you call add(buffer), you're passing the exact same pointer every time (technically, arrays aren't pointers but it's close enough for this discussion). That pointer gets assigned to the data field of your nodes. This means that every node contains a reference to the exact same string. That's why every node is printing out the same value. In addition, as your code becomes more complex, you run the risk of these references becoming invalid if buffer's lifetime ever expires.

What you want to do is use malloc to give each node its own copy of the string.

void add(char *data) {
    size_t len = strlen(data);
    node *new_node = malloc(sizeof(node)); // Note: You should check this return value.
    new_node->data = malloc(len 1); // Note: Ditto.
    memcpy(new_node->data, data, len 1);
    new_node->next = head;
    head = new_node;
    printf("Adding new word:%s\n",data);
}

void free_list(ptr *hnode)
{
    ptr p;
    while(*hnode){
        p = *hnode;
        *hnode = (*hnode)->next;
        free(p->data);
        free(p);
    }
}

CodePudding user response：

In the function add data members data of all nodes point to the same array buffer declared in main

void add(char *data) {
        node *new_node = malloc(sizeof(node));
        new_node->data = data;
        new_node->next = head;
        head = new_node;
    printf("Adding new word:%s\n",data);
}

So the function print_list will output what is last stored in the buffer.

You need to make dynamically a copy of the passed string.

Also this while loop

while(i < MAX-1 && !feof(file))
{
    if((ch = fgetc(file)) == ' '|| ch == '\n')
    {
        buffer[i] = '\0';
        add(buffer);
        i = 0;
    }
    else{
        buffer[i  ] = ch;
    }
}

does not make a great sense. First of all the variable ch shall be declared as having the type int. The condition in the loop

while(i < MAX-1 && !feof(file))

can result in storing the value EOF in the buffer. And the output demonstrates this.

(Edit: this update in your question

       if(ch == EOF)
              break;

you made after my answer.)

Also reading strings by one character is inefficient. You should use the function fscanf instead of fgetc.

Pay attention to that it is a bad idea to define the global variable head

ptr head = NULL;

and when some functions depend on the global variable.

Also in some parts of the program you are using the typedef name ptr while in other parts of the program you are using the type node *. This only confuses readers of the code.