Keeping struct variable in storage in loop?-CodePudding

I have the following code.

#include <stdio.h>
#include <string.h>

struct node
{
    char value[30];
    struct node *next;
};

int main()
{
    struct node *current = NULL;
    struct node *head = NULL;

    while (1)
    {
        char input[30];
        scanf(")s", &input);
        if (strcmp(input, "exit") == 0) break;
        struct node new;
        strcpy(new.value, input);
        if (head == NULL)
        {
            head = &new;
        }
        else
        {
            current->next = &new;
        }
        
        current = &new;
    }
    
    return 0;
}

This is supposed to store multiple inputs of String in a linked list. After a long time I found the problem in this code. The problem is that after each loop the struct variable (list node) is destroyed in storage, and if I initialize a new struct variable it is placed at the same place in the RAM where I had the previous list node. So the only list node that remains is the last one I create. How can I make it so that the list nodes don't get destroyed after each loop run?

Above, I use list node and struct variable interchangeably, because in my program the struct variable represents a list node that is supposed to be created.

CodePudding user response：

Well, you can do it in linear code, but it's easier if you break down the code flow into whatever job each step has to do. Let's start:

We leave the "intro" unchanged, except for adding stdlib:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct node
{
    char value[30];
    node *next;
};

Now we "shorthand" the declaration (yes, yes, macros are evil, but sometimes, they are useful)

// typedefs are a good alternative here if you dislike macros
#define node_t struct node

Let's start with a helper: adding nodes

node_t* CreateNewNode (node_t* prev)
{
    // (type*)malloc(sizeof(type)) is explicit and useful
    // but not mandatory in C; you might need it if you write C  
    // that shares C code though
    // details here: https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc
    node_t* node = (node_t*)malloc(sizeof(node_t));
    node->next = NULL;

    if (prev != NULL)
        prev->next = node;

    return node;
}

What we're basically doing here is allocating memory for new nodes and because it's a linked list, we're keeping the chain going referencing the previous node.

Now let's add some clean-up. We destroy the linked list. (well, we have to, we allocated memory so now we have to deallocate it)

void DestroyNodes (node_t* head)
{
    node_t* current = head;

    while (current != NULL)
    {
         free(current);
         current = current->next;
    }
}

We c-p the entry-point and the first part of it (replacing the struct with the macro)

int main (void)
{
    node_t* current = NULL;
    node_t* head = NULL;

    while (1)
    {
        char input[30];
        scanf(")s", &input);

        if (strcmp(input, "exit") == 0)
            break;

While new is C keyword, and we're playing in C and we showed the compiler that the code has nothing to do with objects, I would really not tempt the compiler gods here, so don't use that as a variable name "just in case".

Now we start using our helpers, along with some different names for the vars:

        node_t* prev = current;
        current = CreateNewNode(prev);
        strcpy(&(current->value)[0], input);    // strcpy(current->value, input); is fine too

        if (head == NULL)
            head = current;
    }

Then the deallocation before returning from main() to avoid memory leaks:

DestroyNodes(head);

So, summarizing:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct node
{
    char value[30];
    node *next;
};

// typedefs are a good alternative here if you dislike macros
#define node_t struct node

node_t* CreateNewNode (node_t* prev)
{
    // (type*)malloc(sizeof(type)) is explicit and useful
    // but not mandatory in C; you might need it if you write C  
    // that shares C code though
    // details here: https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc
    node_t* node = (node_t*)malloc(sizeof(node_t));
    node-> next = NULL;

    if (prev != NULL)
        prev->next = node;

    return node;
}

void DestroyNodes (node_t* head)
{
    node_t* current = head;

    while (current != NULL)
    {
         current = current->next;
         free(current);
    }
}

int main (void)
{
    node_t* current = NULL;
    node_t* head = NULL;

    while (1)
    {
        char input[30];
        scanf(")s", &input);

        if (strcmp(input, "exit") == 0)
            break;

        node_t* prev = current;
        current = CreateNewNode(prev);
        strcpy(&(current->value)[0], input);    // strcpy(current->value, input); is fine too

        if (head == NULL)
            head = current;
    }

    // clean-up
    DestroyNodes(head);
    
    return 0;
}

CodePudding user response：

As mentioned in comments you should use malloc. This is because you want to reserve storage for each new object at the point of creation. Otherwise objects without any linkage (like new in your example) are destroyed at the end of the scope (in the next iteration entering the while loop or at the end of it):

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

struct node
{
    char value[30];
    struct node *next;
};

int main()
{
    struct node *current = NULL;
    struct node *head = NULL;

    while (1)
    {
        char input[30];
        scanf(")s", &input);
        if (strcmp(input, "exit") == 0) break;
        struct node *new = malloc(sizeof *new);
        strcpy(new->value, input);
        if (head == NULL)
        {
            head = new;
        }
        else
        {
            current->next = new;
        }
        
        current = new;
    }
    
    return 0;
}

Of-course this would create memory leakage but that should not be an issue with this small program since the operating system will automatically free the allocations created.