CODEWITHSUNDEEP
c
InsaneCoder
InsaneCoder

Reputation: 8268

Linked List Implementation in C - Segmentation fault error

I am struggling to find the error in the following implementation of linked list.I am getting segmentation fault error when I append or add at beginning

Please Help me where I am doing wrong

#include

#include<stdlib.h>

struct node
{
    int data;
    struct node*link;
};

void append(struct node *p,int num)
{
    struct node *temp,*q;
    q=p;
    temp=(struct node*)malloc(sizeof(struct node));
    temp->data=num;
    temp->link=NULL;
    if(p==NULL)
        p=temp;
    else
    {
        while(q->link!=NULL)
            q=q->link;

        q->link=temp;
    }
}



int main()
{
    int i,choice,num,pos;
    struct node p;
    printf("Enter your choice\n");
    printf("1-Append\n2-Add At Beg\n3-Add after\n4-Delete\n5-Exit");
    scanf("%d",&choice);
    while(choice!=5)
    {
        switch(choice)
        {
            case 1:printf("Enter the number\n");
                    scanf("%d",&num);
                    append(&p,num);
                    break;


        }
        printf("1-Append\n2-Add At Beg\n3-Add after\n4-Delete\n5-Exit");
        scanf("%d",&choice);
    }
}

Upvotes: 0

Views: 305

Answers (3)

ebo
ebo

Reputation: 9215

Final edit: User vaibhav caught it: struct node *p is uninitialized and may not be NULL even if the list is empty.

Just for general enjoyment, the clang static analyser seems to get it:

clang -Wall --analyze lists.c
lists.c:13:5: warning: Assigned value is garbage or undefined
  q = *p;
    ^ ~~
1 warning generated.

void append(struct node *p,int num);

While the algorithms themselves seem fine, there is a problem in the way you handle the p-argument. You pass the pointer p by value, which means changes to *p will change the allocated memory, but changes to p itself will not be propagated to the calling context.

The correct method to handle this case would be:

void append(struct node **p, int num) { *p = temp; }

#include<stdlib.h>

struct node
{
  int data;
  struct node*link;
};

void append(struct node **p,int num)
{
  struct node *temp,*q;
  q = *p;
  temp = (struct node*)malloc(sizeof(struct node));
  temp->data = num;
  temp->link = NULL;
  if(*p == NULL)
    *p = temp;
  else
    {
      while(q->link != NULL)
        q = q->link;

      q->link = temp;
    }
}

int main()
{
  struct node *p;

  append(&p, 1);
  append(&p, 2);
  append(&p, 3);
}

Upvotes: 1

vaibhav
vaibhav

Reputation: 117

do 

struct node*p=NULL;

call append like this-

append(&p,num);

We do this as we want to keep the pointer to the first node of our link list with us. By doing append(p,num), a copy of the pointer goes into our method and the changes to p are lost when that method returns.

and write the append routine as-

void append(struct node **p,int num)
{
   struct node *temp,*q;
   q=*p;
   temp=(struct node*)malloc(sizeof(struct node));
   temp->data=num;
   temp->link=NULL;
   if(*p==NULL)
   {
        *p=temp;
        printf("here");
    }
  else
   {
       while(q->link!=NULL)
        q=q->link;

       q->link=temp;
   }
}

do similarly for add routine.

Upvotes: 3

zubergu
zubergu

Reputation: 3706

In this line:

struct node*p;

it should be

struct node p;

you're creating pointer to node. But don't initialize it later, so no node is created and it points to random location. Then you pass that as argument to your functions.
When you are dereferencig this pointer in your functions p-> you have a segmentation fault.

In your main function you should have declaration of 'node', not pointer to node, and then pass node's address to function with & operator.

So your function declaration void addatbeg(struct node *p,int num); is fine but it should be called (after you declare node p):

addatbeg(&p,some_number);

Also, every function that makes use of p should be changed to treat p like a structure, not like pointer to structure.

What you did here is confused some information on linked lists.
Usually head of a singly linked list declared as a pointer to node. When I implemented sll myself I did that, too. Then you pass that pointer by reference to functions manipulating on linked list and it works fine.
On the other hand you got doubly linked lists, and there it is more convinient to treat head of doubly linked list as just another node, but without value.
You are trying to mix those ideas together without understanding ideas behind them.
I gave you bunch of tips on how to make it work, but fixing it would mean that I have to rewrite your whole code.

And that's not the point since you should learn it yourself.

Upvotes: 0

Related Questions