Error with Double Linked List, I can't find the missing attribute before '*'

Question

C beginner here, trying to learn more about Linked Lists.

The following code is supposed to create a circular doubly linked list from a structure called "soldier". The int n is important since it determines the number of nodes created, each node contains an int data with the value of n, as long as n=>1.

So when the user enters n=6, the linked list will look like:

6 <-> 5 <-> 4 <-> 3 <-> 2 <-> 1
^                             ^
|_ _ _ _ _ _ _ _ _ _ _ _ _ _ _|

I have been stuck here for a while now. I am trying to see what I am missing but can't see it. Everything compiles files, except that I get only the following error: [Error] expected '=', ',', ';', 'asm' or '__attribute__' before '*' token

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

typedef struct nod{
    int data;
    struct nod *prev, *next;
}soldier;

soldier *head;

void soldier* create_soldier (int sequence){
    if(head->data==NULL)    //when the linked list starts
        head->data = sequence;

    else{
        soldier *temp;
        soldier *t;
        temp= (soldier *) malloc(sizeof(soldier));
        temp->data = sequence;
        temp->next = NULL;

        t= head;    //Traversing
        while (t->next != NULL)
            t= t->next;

        if(temp->data==1){      //for the rear end of the array to link back to the head
            t->next = temp;
            temp->prev = t;
            temp->next = head;
            head->prev = temp;
        }
        else{
            t->next = temp; 
            temp->prev = t;
        }
    }
}

void display(soldier* head){
    soldier *t;
    t=head;

    while (t->next != head){
            printf("%d", t->data);
            t= t->next;
    }   
}

void display(soldier* head){
    soldier *t;
    t=head;

    while (t->next != head){
            printf("%d", t->data);
            t= t->next;
    }   
}

int main()
{
    int n, k;
    printf("Enter the number of soldiers to be executed");
    scanf("%d", &n);
    printf("Enter the number of soldiers to be skipped");
    scanf("%d", &k);

    for ( ; n>= 1; n--)
        create_soldier(n);

    display(head);

    return 0;
}

Answer 1

I've spotted a few potential issues:

---

Your prototype for create_soldier has two return types:

void soldier* create_soldier (int sequence){ ... }

You'll have to choose one or the other! I don't see a return in the function, so it should probably be a void function as it currently stands:

void create_soldier (int sequence){ ... }

---

You're comparing an int to NULL here:

if(head->data==NULL)

NULL can only be meaningfully compared against pointers, so you probably intended to compare the pointer to soldier head, and not its data member:

if (head == NULL)

---

void display(soldier* head) is defined twice, so you'll want to remove or rename one definition. They look to be identical to me, so I think you could just remove one.

---

Lastly, don't forget to free the memory you've allocated with malloc once you're done using it. If you don't, you'll end up with a memory leak. It might not matter for a small program, but it's a good habit to pick up early on.

Answer 2

There are a few errors. The return types are not correct, the data initialization gives a warning because you're comparing pointers with ints, but the most important error is that you did not allocated memory to head and that you did not initialize 'head' properly. Also, you should free your memory when done (I did not put that in). I also did not check whether the code does exactly what you want it to do, but the following runs:

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

typedef struct nod {
    int                data;
    struct nod *prev, *next;
} soldier;

soldier *head;

void create_soldier (int sequence) {
    if(head->data == 0) {    // when the linked list starts
        head->data = sequence;
        head->prev = NULL;
        head->next = NULL;
    }

    else{
        soldier *temp;
        soldier *t;
        temp= (soldier *) malloc(sizeof(soldier));
        temp->data = sequence;
        temp->next = NULL;

        t = head;    //Traversing
        while (t->next != NULL)
            t = t->next;

        if(temp->data==1){      //for the rear end of the array to link back to the head
            t->next = temp;
            temp->prev = t;
            temp->next = head;
            head->prev = temp;
        }
        else{
            t->next = temp;
            temp->prev = t;
        }
    }
}

void display(soldier* head){
    soldier *t;
    t=head;

    while (t->next != head){
            printf("%d", t->data);
            t= t->next;
    }
}

int main()
{
    int n, k;
    printf("Enter the number of soldiers to be executed");
    scanf("%d", &n);
    printf("Enter the number of soldiers to be skipped");
    scanf("%d", &k);

    head = (soldier *) malloc(sizeof(soldier));
    for ( ; n>= 1; n--)
        create_soldier(n);

    display(head);

    free(head);
    return 0;
}

Answer 3

Looks like you're solving the Josephus Problem!

There are a variety of compilation issues here to tackle first. Compile your code using flags like

-Wall -Wextra -Werror -O2 -std=c99 -pedantic

if you aren't already. As you code, compile and run often. Use a tool like valgrind to verify that your code doesn't leak memory and help detect segmentation faults.

---

Compiler issues:

• void soldier* create_soldier (int sequence) is an invalid function because it specifies two return types. It should be void create_soldier(int sequence) since it doesn't return anything.
• display is defined twice.
• Warning: if(head->data==NULL) compares an int and NULL; you might want 0 to be a valid data value and the intent was probably if (head == NULL).

Runtime issue:

• (head->data==NULL) starts off the create_solder function, but this immediately dereferences a null pointer.
• Memory is allocated but not freed.

Design issues and style suggestions:

• The global variable head is unnecessary. It should be in the main scope and passed into any function that needs it.
• In keeping with the previous point, these functions are unreusable because their implementation is totally bound to the global variable head, making it impossible to create multiple lists, for example. Try to write pure functions that don't mutate external state. This makes programs less error prone and easier to reason about.
• In keeping with the previous point, prefer to use generic names like node for your code instead of soldier. We want to be able to write a doubly linked list "class" and reuse it for any purpose (such as solving this particular problem). If you want this, just add a typedef soldier alias of the existing node type.
• void create_soldier (int sequence) isn't a very useful function because it's seldom that we need to create lists with data from 1..n. More frequently, we just want to create a node and give it some arbitrary data. With this in mind, I'd prefer void create_node(node **head, int data), which just creates one node. Then we can run a loop in main to create n nodes per the problem specification (or write a wrapper function for create_node that runs the 1..n logic).
• Prefer for loops to while loops. They're much cleaner and keep variables scoped nice and tight.
• No need to cast the result of malloc.

---

Here's a rewrite suggestion:

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

typedef struct node {
    int data;
    struct node *prev, *next;
} node;

void create_node(node **head, int data) {
    if (*head) {
        node *new_node = malloc(sizeof(*new_node));
        new_node->data = data;
        new_node->next = *head;
        new_node->prev = (*head)->prev;
        (*head)->prev->next = new_node;
        (*head)->prev = new_node;
    }
    else {
        *head = malloc(sizeof(**head));
        (*head)->data = data;
        (*head)->prev = *head;
        (*head)->next = *head;
    }
}

void display(node *head) {
    node *t = head;

    if (t) {
        printf("%d->", t->data);

        for (t = t->next; t != head; t = t->next) {
            printf("%d->", t->data);
        }

        puts("");
    }
}

void free_list(node *head) {
    node *t = head;

    if (t) {
        for (t = t->next; t != head;) {
            node *dead_node = t;
            t = t->next;
            free(dead_node);
        }

        free(head);
    }
}

int main() {    
    int n;
    node *head = NULL;
    printf("Enter the number of soldiers to be executed: ");
    scanf("%d", &n);        

    for (int i = 0; i < n; create_node(&head, ++i));

    display(head);
    free_list(head);
    return 0;
}