CODEWITHSUNDEEP
cstructlinked-listsingly-linked-listfunction-definition
kuwira
kuwira

Reputation: 15

linked list problem when displaying the list in C

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();
   
}

Upvotes: 1

Views: 129

Answers (2)

Vlad from Moscow
Vlad from Moscow

Reputation: 310980

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.

Upvotes: 1

B Remmelzwaal
B Remmelzwaal

Reputation: 1629

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

Upvotes: 1

Related Questions