Keeping struct variable in storage in loop?

Question

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("%29s", &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.

You should use dynamic allocation with malloc(), not a local struct. — Barmar
– Barmar, Commented Dec 29, 2022 at 21:33
Also remember to free() what you malloc(). And since it's a linked list (which is "almost" an array), you need to free each member of the list. — TheNomad
– TheNomad, Commented Dec 29, 2022 at 22:17

TheNomad · Accepted Answer · 2022-12-29 22:14:59Z

2

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("%29s", &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("%29s", &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;
}

edited Dec 29, 2022 at 22:14

answered Dec 29, 2022 at 21:55

TheNomad

1,7281 gold badge11 silver badges16 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

mkdrive2 Over a year ago

May I ask why you added (node_t*) before the malloc? Is that necessary? It seems to work without it, so I thought it was a mistake, but it seems to be right, after all?

TheNomad Over a year ago

The idea is that malloc() does what the function name says: it "allocs", meaning it allocates memory needed for it. The memory needed is the size you specify (as a size_t). The coup-de-grace is that because C is so close to low-level, it mostly deals with what's allocated in the memory. It doesn't know what's for. Yeah, you can say sizeof(myType) but that will just translate to 68 (for example, assuming myType is 68 bytes). But it has no idea what myType is or that you need it (it doesn't "remember" it in that context). So I prefer to cast to what the pointer needs to be.

paulsm4 Over a year ago

@mkdrive2: Look here: stackoverflow.com/a/605856/421195: "In C, you don't need to cast the return value of malloc. The pointer to void returned by malloc is automagically converted to the correct type. However, if you want your code to compile with a C++ compiler, a cast is needed." Personally, I prefer the explicit cast. FYI, I would also prefer typedef over "#define node_t struct node".

TheNomad Over a year ago

@paulsm4 I wanted to go down the typedef route, but since I am a C++ fanboy, I felt I'd get too close :) @mkdrive2 if you wanna stick to C, then yes, malloc(sizeof blah) is fine. If you know you'll move on to C++ casting might become something common. Keep in mind that inline casting such as that is usually replaced by static_cast and dynamic_cast when touching C++ (depending on the situation of course... ). I added both points in the code now.

AnArrayOfFunctions · Accepted Answer · 2022-12-29 22:12:41Z

1

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("%29s", &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.

edited Dec 29, 2022 at 22:12

answered Dec 29, 2022 at 21:40

AnArrayOfFunctions

3,9984 gold badges38 silver badges77 bronze badges

5 Comments

mkdrive2 Over a year ago

There seems to be a mistake in your code. It should be malloc(sizeof(struct node)). After I changed that, it worked. Thanks. Do you perhaps know why in question stackoverflow.com/questions/14768230/… the person with the question writes a (struct Vector*) before the malloc?

AnArrayOfFunctions Over a year ago

@mkdrive2 There is no mistake in my code - what compiler are you using?

AnArrayOfFunctions Over a year ago

@mkdrive2 About your second question - stackoverflow.com/questions/605845/…

mkdrive2 Over a year ago

Thanks for the link! I am using gcc and there it does not work the way you wrote it. It does not save the information I want. I left out the code that prints the list afterwards, but I used that to verify.

AnArrayOfFunctions Over a year ago

@mkdrive2 Oh yeah sorry- you need sizeof *new My bad

Collectives™ on Stack Overflow

Keeping struct variable in storage in loop?

2 Answers 2

4 Comments

5 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related