linked list problem when displaying the list in C

Question

having segmentation error while trying to access nodes

i can create new nodes with my add function after function executes i cant access my nodes. i think they deallocated in memory but i couldnt figure it out.

#include <stdio.h>
#include <stdlib.h>
struct node
{
    int data;
    struct node *nextNode;
};
struct node *head;
void add(int data)
{
    

    struct node *new = (struct node *)malloc(sizeof(struct node));
    new->data = data;
    new->nextNode = NULL;
    struct node *temp1;
    temp1 = head;
    
    while (temp1 != NULL)
    {
        temp1 = temp1->nextNode;
    }

    temp1 = new;
    printf("\nValue of temp1:%d\nValue of new: %d\n",temp1,new);
    printf("\nData of temp1:%d\nData of new:%d\n",temp1->data,new->data);
}
void printList()
{
    int i = 1;
    struct node *tempP;
    tempP = head;
    while (tempP != NULL)
    {
        printf("\nData of %dth element is : %d\n", i, tempP->data);
        tempP = tempP->nextNode;
        i++;
    }
}

void main()
{
    head = (struct node *)malloc(sizeof(struct node));
    head->data = 10;
    head->nextNode = NULL;
    add(20);
    add(30);
    add(40);
    printList();
   
}

`while (temp1 != NULL)` guarantees `temp1` to _be_ `NULL`. Change that to `while (temp1->nextNode != NULL)` to get the last node in your list. — B Remmelzwaal, Feb 09 '23 at 14:47
I'm also curious what the "value" print statements should do. Do you want them to be the addresses of the nodes? If so, don't use `%d` which is for ints, use `%p` for pointers. — B Remmelzwaal, Feb 09 '23 at 14:50
@BRemmelzwaal i wrote value print statements because i want to see does my add function really works. You said that i should change the while value but when i tried it, the printList function didnt worked. my question is when i try call printList function, it prints the head but nothing else. But in add function when i try to print the list it prints whole list. Does my list deletes after the function executes? — kuwira, Feb 09 '23 at 15:00

B Remmelzwaal · Answer 1 · 2023-02-09T15:08:58.373

There were a handful of mistakes in your add() function, which I've highlighted and fixed below:

void add(int data)
{
    struct node *new = malloc(sizeof(*new));  // suggested by ryyker
    new->data = data;
    new->nextNode = NULL;
    struct node *temp1 = head;  // just keep it short
    
    while (temp1->nextNode != NULL)  // temp1 != NULL will always result in it being NULL, last node is the node with NULL as next
    {
        temp1 = temp1->nextNode;
    }

    temp1->nextNode = new;  // you want the next in the list to be the new node, not reassign the head to a new node. That's a memory leak.
                            // remember: temp1 == head, and head = new makes head lose the original node and point to the newly created one

    printf("\nValue of temp1:%p\nValue of new: %p\n",temp1,new);  // %p for pointers
    printf("\nData of temp1:%d\nData of new:%d\n",temp1->data,new->data);
}

Output:

Value of temp1:0x55809a4b22a0
Value of new: 0x55809a4b22c0

Data of temp1:10
Data of new:20

Value of temp1:0x55809a4b22c0
Value of new: 0x55809a4b26f0

Data of temp1:20
Data of new:30

Value of temp1:0x55809a4b26f0
Value of new: 0x55809a4b2710

Data of temp1:30
Data of new:40

Data of 1th element is : 10

Data of 2th element is : 20

Data of 3th element is : 30

Data of 4th element is : 40

+1, with suggestion: `struct node *new = (struct node *)malloc(sizeof(struct node));` could also be shortened to `struct node *new = malloc(sizeof(*new));`. (note that casting return of `malloc` and family is not recommended in `C`) — ryyker, Feb 09 '23 at 15:07
Personally not _too_ familiar with the way C handles memory allocations, so thanks for the suggestion :) — B Remmelzwaal, Feb 09 '23 at 15:09
[This might be an enjoyable read](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) for you then. It is not all one sided, but I tend to fall on the side of not casting as I have first hand experience with problems in code where the cast was used everywhere. — ryyker, Feb 09 '23 at 15:12

Vlad from Moscow · Accepted Answer · 2023-02-09T15:27:18.627

This code snippet within the function add

struct node *temp1;
temp1 = head;

while (temp1 != NULL)
{
    temp1 = temp1->nextNode;
}

temp1 = new;

is wrong. Within it there is changed the local variable temp1. It is not linked with the list.

Also using the conversion specifier %d to output a pointer invokes undefined behavior. You should use conversion specifier %p.

Using your approach to the function definition you could write instead.

void add(int data)
{
    struct node *new = malloc( sizeof( *new ) );
    new->data = data;
    new->nextNode = NULL;

    if ( head == NULL )
    {
        head = new;
    }
    else
    {
        struct node *temp1 = head;
    
        while ( temp1->nextNode != NULL)
        {
            temp1 = temp1->nextNode;
        }

        temp1->nextNode = new;
    }

    printf("\nValue of temp1->nextNode:%p\nValue of new: %p\n", 
           ( void * )temp1->nextNode, ( void * )new);
    printf("\nData of temp1->nectNode:%d\nData of new:%d\n",
            temp1->nextNode->data,new->data);
}

Pay attention to that it is a bad design when functions depend on a global variable as in your case where the functions depend on the global variable head.

And it is also a bad idea when the first node is added to the list bypassing the function add.

And you need check whether a node was successfully allocated.

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

int main( void )

As for me I would declare the pointer to the head node in main like

int main( void )
{
    struct node *head = NULL;
    // ...

And the function add will look like

int add( struct node **head, int data )
{
    struct node *new_node = malloc( sizeof( *new_node ) );
    int success = new_node != NULL;

    if ( success )
    {
        new_node->data = data;
        new_node->nextNode = NULL;

        while ( *head != NULL ) head = &( *head )->nextNode;

        *head = new_node;
    }

    return success;
}

and called like

struct node *head = NULL;

add( &head, 10 );
add( &head, 20 );
add( &head, 30 );
add( &head, 40 );

In turn the function printList can look like

void printList( const struct node *head )
{
    for ( size_t i = 1; head != NULL; head = head->nextNode )
    {
        printf( "Data of %zuth element is : %d\n", i++, head->data);
    }
}

And you need at least to write one more function that will free all the allocated memory.

I like the improvement of adding a `head` case in the add function, because not only is the assignment of it in main duplicate code, but a crash is inevitable once a delete function is implemented and `head` gets deleted. — B Remmelzwaal, Feb 09 '23 at 15:07

linked list problem when displaying the list in C

2 Answers2