0

I have a simple singly linked list, which I'm trying to step through and print the name of the data at each node, in this case the nodes hold fruits which contain a single field, name:

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

struct store
{
    struct list * inventory;
};

struct list
{
    struct node * start; 
    int length;
};

struct node 
{
    struct fruit * data;
    struct node * next;
};

struct fruit 
{
    char name[20];
};

/* set inventory to default vals */
void store_init(struct store * store)
{
    store -> inventory = malloc(sizeof(struct list)); 
    store -> inventory -> start = NULL;
    store -> inventory -> length = 0;   
}

void open_store(struct store * store)
{
    struct node * current;
    struct node * traversal_node;
    int i;
    struct fruit fruits[3];  
    char * fruit_names[3];
    fruit_names[0] = "Apple";
    fruit_names[1] = "Mango";
    fruit_names[2] = "Orange"; 

    /* populate fruits with relevant data */
    for(i=0; i < 3; i++)
    {
        strcpy(fruits[i].name, fruit_names[i]);
    }

    for(i=0; i < 3; i++) 
    {
        current = malloc(sizeof(struct node));

        current -> data = &fruits[i];
        current -> next = NULL;

        if(store -> inventory -> start == NULL) { /* check if no other nodes have been inserted, if so, insert at start */
            store -> inventory -> start = malloc(sizeof(struct node));
            store -> inventory -> start = current;
            store -> inventory -> length++;
        } else  {
            traversal_node = store -> inventory -> start;    /* start at header */

            while(traversal_node->next != NULL) {  /* move to end of list */
                traversal_node = traversal_node -> next;
            }

            traversal_node -> next = current;           /* add node */
            store -> inventory -> length++;
        }       

    }

    printf("%s\n", store -> inventory -> start -> data -> name);

}

void print_inventory(struct store * store)
{
    printf("%s\n", store -> inventory -> start -> data -> name);
}

int main()
{
    struct store store;

    /* intiatlize store */
    store_init(&store);

    open_store(&store);

    print_inventory(&store);

}

The first print statement works, but I get nothing when I try calling print_inventory, and if I try looping through the inventory in that function, I get random characters.

I'll note if I try printf("%s\n", store.inventory -> start -> data -> name); in main, it works fine, but I can't seem to pass my store into the print_inventory function.

What am I doing wrong?

  • in my opinion it is difficult to read if you left spaces left and right of the `->` operator. And if you decide to keep this spaces it would be consequent to also put these spaces left and right of the `.` – vlad_tepesch Oct 13 '14 at 09:01

2 Answers2

1

your fruits in the open_store function is an automatic (stack) variable. Later in that function you add its pointer to your linked list.

Then the routine is exited the fruits array goes out of scope and pointers to it become invalid and its content will be overwritten sooner or later. So then you access the pointer of your linked list nodes you read random memory.

You have to allocate dynamic memory for the list data.

    current -> data = malloc(sizeof(struct fruit));
    strcpy(current -> data -> name, fruit_names[i]);
vlad_tepesch
  • 6,681
  • 1
  • 38
  • 80
  • But I'm still able to access the values assigned to my list in `main`? How am I able to do that if the memory for `fruits` goes out of scope at the end of the function? –  Oct 13 '14 at 09:11
  • The memory of course is still there. And if its not required it will not be overwritten. But if you enter another sub routine its parameter and local variables will be placed at the former location of `fruites` and overwrites its content. Please make yourself familiar with the concept of the stack memory and its usage. – vlad_tepesch Oct 13 '14 at 09:14
  • 1
    And to do so, you can check out http://stackoverflow.com/questions/79923/what-and-where-are-the-stack-and-heap – Dettorer Oct 13 '14 at 09:18
  • Don't declare your fruits as an array. Use malloc to initialise each fruit and then it'll work. Like @vlad_tepesch said, you're pointing to a variable on the stack which goes out of scope. – Yusuf Jakoet Oct 13 '14 at 09:20
  • Okay, I'll look into it. Thanks! And thank you @Dettorer for the link, that looks like a good place to start. –  Oct 13 '14 at 09:20
1

This brings back so many memories! There are a lot of problems with your code, but to make it run, you'll need to fix two things:

1) Make sure that the fruits that you load in the inventory are available whenever you need them. The way you have arranged things, it won't happen. This is because the fruits are dead as soon as open_store returns. This is because they live on the stack of the function, which is kaput as soon as the function returns to the caller. There is a rare chance that you might be able to access them, but that would be relying on an undefined behavior. To fix this, you'll need to do the following:

    struct fruit *fruits[3];  
    char * fruit_names[] = {"Apple", "Mango", "Orange"};
    for(i=0; i < 3; i++) {
        fruits[i] = malloc(sizeof(struct fruit));
        strcpy(fruits[i]->name, fruit_names[i]);
    }

With this change, the fruits will be created in the heap (do familiarize yourself with these concepts if you aren't already), and will be available nice and fresh whenever you need them (as long as the program (process) is running). You'll also need to change:

current -> data = fruits[i]; //& is now gone

And we are done.

2) However, the fruits still won't show up. This is because your print_inventory has a strange implementation, to put it mildly.

void print_inventory(struct store * store)
{
    struct node *item = store -> inventory->start;
    int i;
    for(i = 0 ; i < store -> inventory -> length; i++)  {
        printf("%s\n", item -> data -> name);
        item = item -> next;
    }
}

Should work.

axiom
  • 8,765
  • 3
  • 36
  • 38