CODEWITHSUNDEEP
cstringpointersqueue
Mr. Adobo
Mr. Adobo

Reputation: 835

Queue struct losing element values after being called once

I am writing a queue struct in C that takes strings (char pointers) as the element. The problem I'm having is that after printing the element value once, it comes out null afterwards.

With the code below, I am expecting this output:

1 Hello
1 Hello
2 World
Hello World
1

But I am getting this output:

1 Hello
1 (null)
2 World
(null) (null)
1

Can someone enlighten me on what I'm doing wrong?

#include <stdlib.h>
#include <stdio.h>

struct Node {
    struct Node *next;
    char* element; 
};
struct Queue {
    int size;
    struct Node *head;
    struct Node *tail;

};

void Enqueue(struct Queue *q, char* str) {
    struct Node newNode = {0,str};
    if(q->size == 0) {
        q->head = &newNode;
    }
    else {
        q->tail->next = &newNode;   
    }
    q->tail = &newNode;
    q->size = q->size + 1;
}
char* Dequeue(struct Queue *q) {
    if(q->size < 0) {   
        return -1;
    }
    char* tbr = q->head->element;
    struct Node* oldNode = q->head;
    q->head = oldNode->next;
    q->size = q->size - 1;
    if(q->size == 0) {
        q->tail = NULL;
    }
    return tbr;
}
int IsEmpty(struct Queue *q) {
    return q->size == 0;
}
char* Peek(struct Queue *q) {
    return q->head->element;
}
int main() {
    struct Queue q = {0};
    Enqueue(&q,"Hello");
    printf("%d %s\n",q.size,q.head->element);
    printf("%d %s\n",q.size,q.head->element);
    Enqueue(&q,"World");
    printf("%d %s\n",q.size,q.head->next->element);
    printf("%s %s\n",Dequeue(&q),Dequeue(&q));
    printf("%d\n",IsEmpty(&q));
    printf("%s %s\n","Hello","World");
    Dequeue(&q);
    return 0;
}

Upvotes: 0

Views: 268

Answers (2)

Filipe Gon&#231;alves
Filipe Gon&#231;alves

Reputation: 21223

Inside Enqueue, you can't use a local variable to insert a new node in the queue, because you're going to be using it outside of the function's lifetime, and local variables are destroyed after the function returns.

Thus, when you come back from Enqueue, the element inserted (newNode) points to an invalid memory location which will most likely be overwritten when you call another function. All bets are off. You must use dynamic allocation if you want your nodes to live longer:

void Enqueue(struct Queue *q, char* str) {
    struct Node *newNode = malloc(sizeof(struct Node));
    newNode->next = NULL;
    newNode->element = str;
    if(q->size == 0) {
        q->head = newNode;
    }
    else {
        q->tail->next = newNode;   
    }
    q->tail = newNode;
    q->size = q->size + 1;
}

Also, note that now you need to free a node when you dequeue it. Dequeue becomes:

char* Dequeue(struct Queue *q) {
    if(q->size < 0) {   
        return -1;
    }
    char* tbr = q->head->element;
    struct Node* oldNode = q->head;
    q->head = oldNode->next;
    q->size = q->size - 1;
    if(q->size == 0) {
        q->tail = NULL;
    }
    free(oldNode);
    return tbr;
}

Inside main, your final Dequeue will cause a segmentation fault, because the queue is empty, and you don't check for this case in Dequeue. Either you don't call Dequeue when IsEmpty returns true, or you add code to check for this case in Dequeue.

Final note: in main, this line:

printf("%s %s\n",Dequeue(&q),Dequeue(&q));

Will not necessarily print Hello World, because arguments evaluation order is implementation defined. In my machine, for example, it prints World Hello.

Upvotes: 1

alk
alk

Reputation: 70971

Enqueue() inserts references to memory on the stack, which is valid only locally:

void Enqueue(struct Queue *q, char* str) {
  struct Node newNode = {0,str};

As soon as the function returns this storage is freed, and accessing provoke undefine behaviour.

To resolve this dynamically allocate memory from the heap:

void Enqueue(struct Queue *q, char* str) {
  struct Node * pnewNode = calloc(1, sizeof(*pnewNode);
  if (NULL == pnewNode)
  {
    perror("calloc() failed");
    exit(EXIT_FAILURE);
  } 

  pnewNode->element = str;

  if(q->size == 0) {
    q->head = pnewNode;
  }
  else {
    q->tail->next = pnewNode;   
  }
  q->tail = pnewNode;
  q->size = q->size + 1;
}

Note that dynamically allocated memory needs to be free()ed.

Upvotes: 1

Related Questions