patterncMinor
Quick linked-list implementation
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.
It doesn't have to be super fancy or anything. It just has to properly carry out the four functions.
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
Pointer declarations
Remember that the
Now the error (and fix) is obvious. If there is no
Edit: remove
In order to modify the
Then you would call the code like this:
Note: don't forget to check for
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 tostruct 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 caseIn 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.