HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Quick linked-list implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listimplementationlinkedquick

Problem

I need a simple singly-linked list to help implement some memory management functionality. I just finished writing it up and would really like a code review since I haven't written this particular data structure in a long time.

struct pid_node {

  int PID;
  struct pid_node* next; 

};

 struct pid_node* pid_node_create(int PID) {
   struct pid_node* new;
   new = kmalloc(sizeof(struct pid_node));
   if (new == NULL) return NULL;
   new->PID = PID;
   new->next = NULL;
   return new; 
 }

 void add_pid_node(struct pid_node* head, struct pid_node* new) {
   struct pid_node* temp;
   temp = head;
   while(temp->next != NULL) temp = temp->next; 
   temp->next = new; 
 }

 void remove_pid_node(struct pid_node* head, struct pid_node* dead) {
   struct pid_node* temp, other_part_of_list, delete_node;
   temp = head;
   while(temp->next != NULL) {
     if (temp->next == dead) {
       delete_node = temp->next;
       other_part_of_list = temp->next->next;
       temp->next = other_part_of_list;
       kfree(delete_node); //don't leak memory
       return; 
     }
     temp = temp->next; 
   }
   kprintf("Got to end of PID list, didn't remove 'dead'!\n");
 }

 //returns true or false (1 or 0) if a particular PID is within my list
 int query_pid(int PID, struct pid_node* head) {
   struct pid_node* temp;
   temp = head;
   while(temp->next != NULL) {
     if (temp->PID == PID) return 1;
     temp = temp->next;
   }
   return 0; //didn't find it
  }


It doesn't have to be super fancy or anything. It just has to properly carry out the four functions.

Solution

A couple of conceptual errors:

Pass by value

You can remove the temporary copy of head in your functions. In C, all arguments are passed by value. This means you can operate directly on the head parameter exposed to your function without worrying about the argument in the calling code. Although both point to the same location, each is a different pointer variable with its own address.

Pointer declarations

struct pid_node* temp, other_part_of_list, delete_node;


Remember that the is not part of the type, but part of the declarator. A clearer way to write declarations involving pointers is to move the directly in front of the identifier. This follows the C convention that "declaration mimics use." So, the line changes to

struct pid_node *temp, other_part_of_list, delete_node;


Now the error (and fix) is obvious. If there is no * before a variable, its a variable of the type instead of a pointer to the type.

struct pid_node *temp, *other_part_of_list, *delete_node;


Edit: remove head special case

In order to modify the head argument itself, which is a pointer-to-struct, you need to pass a pointer-to-pointer-to-struct or return the new head. For example (pointer-to-pointer-to-struct):

void remove_pid_node(struct pid_node **head, struct pid_node *dead) {
...
    if (*head == dead) {
         temp = *head;
         *head = temp->next; 
         kfree(temp);
         return; 
    }


Then you would call the code like this:

remove_pid_node(&head, head);


Note: don't forget to check for NULL arguments.

Code Snippets

struct pid_node* temp, other_part_of_list, delete_node;
struct pid_node *temp, other_part_of_list, delete_node;
struct pid_node *temp, *other_part_of_list, *delete_node;
void remove_pid_node(struct pid_node **head, struct pid_node *dead) {
...
    if (*head == dead) {
         temp = *head;
         *head = temp->next; 
         kfree(temp);
         return; 
    }
remove_pid_node(&head, head);

Context

StackExchange Code Review Q#10731, answer score: 4

Revisions (0)

No revisions yet.