Solved

Problem with queue in C

Posted on 2012-04-12
7
476 Views
Last Modified: 2012-05-02
i tried to created a queue in c but when i run the code i saw that a  segmentation fault is appear.
What is going wrong?

The code is :

typedef struct node
      {     char name[MAX];
            int id;
            int size_node;
            struct node *next;
      }NODE;

typedef struct
      {
            NODE *head;
            NODE *tail;
      }QUEUE;

void InitialiseQueue(NODE q);
void add(NODE *q,char name[],int id);
void display(NODE *);

int main(void)
{
NODE *q;
InitialiseQueue(*q);
add(q,"A1",11);
return 0;
}


void InitialiseQueue(NODE q)
            {
            NODE *head=NULL;
            NODE *tail=NULL;
            }
void add(NODE *q,char name[],int Identity,int time,char Target[])
      {
       NODE *temp;
        temp = q;
              if(q==NULL)
                    {
                      q=(NODE *)malloc(sizeof(NODE));
                      temp = q;
                    }
            else
                    {
                      while((temp=temp->next)!=NULL);
                      temp->next = (NODE *)malloc(sizeof(NODE));
                      temp=temp->next;
                    }
                     memcpy(temp->name,name,MAX);
                    temp->id=id;
                    temp->size_node=temp->size_node+1;
                    temp->next  = NULL;
      }    
      

void display(NODE *pt)
{
  while(pt!=NULL)
  {
    printf("element is");
    printf("%s\n",pt->name);
    pt=pt->next;
  }
}
0
Comment
Question by:Tom3333
7 Comments
 
LVL 15

Expert Comment

by:Minh Võ Công
ID: 37840937
You change:
int main(void)
{
QUEUE *q = new QUEUE();
q->head = NULL;
q->tail= NULL;
add(q,"A1",11);
/* free memory before return
*/
return 0;
}


void add(QUEUE *q,char name[],int Identity,int time,char Target[])
 {
       NODE *temp= (NODE *)malloc(sizeof(NODE));
       memcpy(temp->name,name,MAX);
       temp->id=id;
       temp->size_node=temp->size_node+1;
       temp->next  = NULL;
       if (q->head == NULL)
       {
           q->head = tmp;
           q->tail = tmp;
       }else {
            q->tail->next = tmp;
            q->tail = tmp;
      }
                 
}
0
 
LVL 7

Expert Comment

by:tampnic
ID: 37842182
If this is not for an academic exercise you should really be using the STL containers (see http://www.cplusplus.com/reference/stl/queue/) to implement a queue. The STL code has been across many many programmers eyes  and utilised in thousands of applications - you couldn't write more reliable and verifiable code by yourself!

Cheers,
   Chris
0
 

Author Comment

by:Tom3333
ID: 37842251
What is the function new QUEUE(); how to define this function???
0
Live: Real-Time Solutions, Start Here

Receive instant 1:1 support from technology experts, using our real-time conversation and whiteboard interface. Your first 5 minutes are always free.

 
LVL 7

Expert Comment

by:tampnic
ID: 37842337
We seem to have an issue with the zones for this question - its been put in the C++ zone so I have given a C++ answer (use the STL). the new function is also C++, in ANSI C you would say
 QUEUE *q = malloc(sizeof(QUEUE));

Open in new window

 or use an operating-system-specific memory allocation routine if suitable

Cheers,
   Chris
0
 
LVL 33

Expert Comment

by:sarabande
ID: 37843833
in C you would pass the QUEUE by pointer to InitializeQueue:


void InitialiseQueue(QUEUE * pq)
{
      pq->head=NULL;
      pq->tail=NULL;
}

Open in new window


in main you would create a QUEUE by

int main()
{
      QUEUE q;
      InitialiseQueue(&q);
      ...
}

and then pass to InitialiseQueue by pointer using the & .

the working add function for C was posted by minhvc though i would use strncpy and not memcpy.

size_t len = strlen(name);
if (len >=  sizeof(temp->name))
   len = sizeof(temp->name) -1;
strncpy(temp->name, name, len);
temp->name[len] = '\0';

Open in new window


Sara
0
 
LVL 22

Expert Comment

by:ambience
ID: 37844448
Few points:

1) You may want to put size_node in QUEUE rather than NODE (I judging that given your code)

typedef struct node
      {     char name[MAX];
            int id;
            struct node *next;
      }NODE;

typedef struct
      {
            NODE *head;
            NODE *tail;
            int size_node;
      }QUEUE;

2) Add wouldnt do what its supposed to. Also when it comes to queues, generally (as convention) you use enqueue and dequeue (but add/remove are OK too). I would suggest that you pass the QUEUE to add

3) add will segfault for sure - see bolded lines

void add(NODE *q,char name[],int Identity,int time,char Target[])
      {
       NODE *temp;
        temp = q;
              if(q==NULL)
                    {
                      q=(NODE *)malloc(sizeof(NODE)); // memory leak as soon as the method exits
                      temp = q;
                    }
            else
                    {
                      while((temp=temp->next)!=NULL);
                      temp->next = (NODE *)malloc(sizeof(NODE)); // temp will always be NULL here
                      temp=temp->next; // whats that for? Will blow up code following this
                    }
                    memcpy(temp->name,name,MAX); // if name is NULL it will blowup
                    temp->id=id;
                    temp->size_node=temp->size_node+1; // this should be in QUEUE
                    temp->next  = NULL;
      }
0
 
LVL 22

Accepted Solution

by:
ambience earned 50 total points
ID: 37844486
Also,

while((temp=temp->next)!=NULL);

beats the purpose of tail.

I recommend you to fatten the following boilerplate

NODE* makeNewNode(const char* name. int identity, int time, const char* target)
{
      // Add validation here, check for null name and target and return if null
      // allocate new node, set data and return
}

void enqueue(QUEUE* queue, const char* name. int identity, int time, const char* target)
{
// Add validation here, check for null queue
NODE* newNode =makeNode(name, identity, time, target);
// Add error check here for NULL node

if(queue->head == NULL)
{
      // update head and tail and set both to same new node
}
else
{
     // update tail - head remains unchanged
}

// update queue node_size
}

NODE* dequeue(QUEUE* queue)
{
// Add validation here, check for null queue
NODE* temp = queue->head;
// Advance head, if head is NULL then set tail NULL too
return temp;
}
0

Featured Post

Courses: Start Training Online With Pros, Today

Brush up on the basics or master the advanced techniques required to earn essential industry certifications, with Courses. Enroll in a course and start learning today. Training topics range from Android App Dev to the Xen Virtualization Platform.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Iteration: Iteration is repetition of a process. A student who goes to school repeats the process of going to school everyday until graduation. We go to grocery store at least once or twice a month to buy products. We repeat this process every mont…
This is a short and sweet, but (hopefully) to the point article. There seems to be some fundamental misunderstanding about the function prototype for the "main" function in C and C++, more specifically what type this function should return. I see so…
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use nested-loops in the C programming language.
The viewer will learn how to use the return statement in functions in C++. The video will also teach the user how to pass data to a function and have the function return data back for further processing.

813 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

14 Experts available now in Live!

Get 1:1 Help Now