CODEWITHSUNDEEP
c++linked-listsegmentation-faultsingly-linked-list
Sayanto Roy
Sayanto Roy

Reputation: 129

Segmentation Fault (Core Dumped) in C++ with linked List basic implementation

I know there are many questions that are based on segmentation fault core dumped in C++ in here but I am not able to resolve this error. My code is simple to create a link list and print it. I want to know why it is wrong because I am new to this topic and want to learn more and how it works and what errors means.

The code is below:


using namespace std;
struct node
{
    int data;
    node *next;
};

node *head =NULL;
void addnode(int x)
{
    node *temp =new node;
    temp->data = x;
    temp ->next = NULL;
    if(head==NULL)
    {
        head = temp;
    }
    else
    {
        node *p =head;
        while(p!=NULL)
        {
            p=p->next;
        }
        p->next =temp;
        temp =NULL;
    }
}

void display(node *head)
{
    node *temp=new node;
    temp = head;
    while(temp!=NULL)
    {
        cout<<temp->data<<" , ";
        temp = temp->next;
    }
    cout<<endl;
}

int main()
{
    cout<<"Hello World";
    node work;
    addnode(12);
    addnode(11);
    addnode(1);
    addnode(22);
    display(head);

    return 0;
}

Most probably I am messing with the head pointer and that's why such an error is occurring but I want to know with respect of my code what I am doing wrong in here.

Thank You. Much Appreciated .

Upvotes: 0

Views: 242

Answers (2)

Vlad from Moscow
Vlad from Moscow

Reputation: 310990

For starters this declaration

node work;

does not make sense. You already declared a pointer to the head node.

node *head =NULL;

In the function addnode in this code snippet

else
{
    node *p =head;
    while(p!=NULL)
    {
        p=p->next;
    }
    p->next =temp;
    temp =NULL;
}

the loop

    while(p!=NULL)
    {
        p=p->next;
    }

is executed until p is equal to NULL. So using a null-pointer in the next statement

    p->next =temp;

results in undefined behavior.

This code snippet should look like

else
{
    node *p = head;
    while( p->next != NULL )
    {
        p = p->next;
    }
    p->next = temp;
}

The function display has a memory leak because a memory at first is allocated and its address is assigned to the pointer temp and then the pointer is reassigned. So the allocated memory will not be deleted.

node *temp=new node;
temp = head;

The function can look like

void display(node *head)
{
    for( node *temp = head; temp !=NULL; temp = temp->next )
    {
        cout<<temp->data<<" , ";
    }
    cout<<endl;
}

Or even like

void display( const node *head )
{
    for( const node *temp = head; temp !=NULL; temp = temp->next )
    {
        cout<<temp->data<<" , ";
    }
    cout<<endl;
}

In any case the list interface is inconsistent. The function addnode deals with the global variable head while the function display gets a parameter. It is a bad idea to use a global variable especially when functions depend on such a variable.

Upvotes: 2

R Sahu
R Sahu

Reputation: 206607

Use of

while(p!=NULL)
{
    p=p->next;
}
p->next =temp;

is a problem. When the loop breaks, p is a NULL pointer. After that, p->next is not good. You need to use:

while (p->next != NULL)
{
    p = p->next;
}
p->next = temp;

Upvotes: 2

Related Questions