CODEWITHSUNDEEP
clinked-listsingly-linked-list
Reda Drissi
Reda Drissi

Reputation: 1672

C - function that creates linked list only returns first node

I have found other questions that were similar but not quite exactly the same, if I'm mistaken please leave a link :)

I've been trying to implement a shell with C and while parsing with pipes in mind I thought about using a linked list of char** args .

My parsing function has issues returning the entire list. I use a tmp node to keep moving while creating new nodes, but when I want to return the original head, it's next is NULL, I thought the pointer tmp to my head was supposed to be just a pointer and that changes have to be made on my head.

Here is simplified code with only the issue.

#include <stdio.h>
#include <stdlib.h>
typedef struct node
{
    int data;
    struct node* next;
} node ;
node* foo()
{
    node* head=malloc(sizeof(node));
    node* tmp=head;
    int i=0;
    for(i=0;i<5;i++)
    {
        tmp=tmp->next;
        tmp=malloc(sizeof(node));
        tmp->data=i;
    }
    return head;
}
int main()
{
    node* list=foo();
    while(list)
    {
        printf("this is your %d\n",list->data);
        list=list->next;
    }
}

if you could just point me in the right direction or tell me what I'm getting wrong that'll be great.

Upvotes: 2

Views: 1832

Answers (3)

Vlad from Moscow
Vlad from Moscow

Reputation: 310930

The function does not make sense because the data member next of each allocated node is not initialized. It is the variable tmp that is being changed in the loop.

The function can look the following way

node* foo()
{
    const int N = 5;
    node *head = NULL;

    node **current = &head;

    for ( int i = 0; i < N; i++ )
    {
        *current = malloc(sizeof( node ) );

        if ( *current )
        {
            ( *current )->data = i;
            ( *current )->next = NULL;
            current = &( *current )->next;
        }
    }

    return head;
}

Instead of using the magic number 5 it is better to specify how many nodes should be created in the list and the initial value as function parameters.

For example

node* foo( size_t n, int init_value )
{
    node *head = NULL;

    node **current = &head;

    for ( size_t i = 0; i < n; i++ )
    {
        *current = malloc(sizeof( node ) );

        if ( *current )
        {
            ( *current )->data = init_value++;
            ( *current )->next = NULL;
            current = &( *current )->next;
        }
    }

    return head;
}

Take into account that in main the pointer list is overwritten in the loop. So you will not be able to access the list one more because the address of the head node will be lost. Use an intermediate variable. For example

int main( void )
{
    node *list = foo();
    for ( node *current = list; current != NULL; current = current->next )
    {
        printf("this is your %d\n", current->data);
    }
}

And according to the C Standard the function main without parameters shall be declared like

int main( void )

Upvotes: 1

Lukingan
Lukingan

Reputation: 84

You need to malloc tmp->next before assigning it to tmp, like this:

for(i=0;i<5;i++)
{
    tmp->next = malloc(sizeof(node));
    tmp=tmp->next;
    tmp->data=i;
    tmp->next = NULL;
}

This outputs:

this is your 0
this is your 0
this is your 1
this is your 2
this is your 3
this is your 4

Upvotes: 0

sudha
sudha

Reputation: 193

node* head=malloc(sizeof(node));
node* tmp=head;
int i=0;
for(i=0;i<5;i++)
{
    tmp=tmp->next;
    tmp=malloc(sizeof(node));
    tmp->data=i;
}
return head;

Here you have created head and gave space, but you have never initialized it and returning it before you initialized it

Upvotes: 1

Related Questions