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

Question

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 .

while (p != NULL) { } - what will the value of p be when the loop terminates? What then happens on the line that follows, p->next = temp? — 1201ProgramAlarm
– 1201ProgramAlarm, Commented Jan 29, 2020 at 19:25
@1201ProgramAlarm What I tried to do is to insert at the end of the linked list. So the while loop is traversing to the last node of the link list and then at the last node which becomes p at the end of while we link using p->next=temp — Sayanto Roy
– Sayanto Roy, Commented Jan 29, 2020 at 19:28
@Chaos problem is p does not become end of the list but one behind it - null pointer. Btw you should use nullptr in C++ instead of NULL. And use ctor to initialize your data - it will make your code cleaner and less error prone. — Slava
– Slava, Commented Jan 29, 2020 at 19:30
Unrelated: Here is an easier (but conceptually a bit harder) way to do insertions to a linked list. In your case you're only looking for the end of the list, so you can ignore the && (*curr)->data <= item test for the ordered insertion point. — user4581301
– user4581301, Commented Jan 29, 2020 at 20:15

R Sahu · Accepted Answer · 2020-01-29 19:27:28Z

2

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;

answered Jan 29, 2020 at 19:27

R Sahu

207k15 gold badges168 silver badges291 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Sayanto Roy Over a year ago

OK. So it worked and Thank You. But now I have another thing to ask....

Sayanto Roy Over a year ago

What if I want to make a head pointer in my main instead of global variable>? That will require to be used as arguments but won't that would be a good implementation in terms of secure coding??

Slava Over a year ago

@Chaos proper coding would be to put your list implementation into a class and put that head pointer to be a private member of that class.

R Sahu Over a year ago

@Chaos, I agree with Slava. User code shouldn't have to deal with the abstraction of the nodes of a list. They should deal with just the list abstraction.

Sayanto Roy Over a year ago

@Slava OK I get that now. Thanks.

Vlad from Moscow · Accepted Answer · 2020-01-29 19:37:51Z

2

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.

edited Jan 29, 2020 at 19:37

answered Jan 29, 2020 at 19:31

Vlad from Moscow

313k27 gold badges204 silver badges358 bronze badges

2 Comments

Slava Over a year ago

Probably you should not show NULL in your corrected code

Sayanto Roy Over a year ago

@Slava Yes I got that right... It worked when I implemented it using the p-next!=NULL

Collectives™ on Stack Overflow

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

2 Answers 2

5 Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related