patterncModerate
Doubly linked list with no apparent memory leaks
Viewed 0 times
withapparentdoublyleaksmemorylistlinked
Problem
I've run my code through Valgrind and managed to not get any memory leaks based on the test code in main. I'm looking for things that I may not have thought to check for, and ways to improve my code in general.
Header file:
Implementation:
```
#include
#include
#include
#include
#include
#include "double_linked_list.h"
struct node *create_empty_node(){
struct node temp = (struct node ) malloc(sizeof(struct node));
temp->next = temp->prev = NULL;
temp->data = NULL;
return temp;
}
struct node create_node(void key){
void *
Header file:
#ifndef DLL_H
#define DLL_H
struct node{
void *data;
struct node *next;
struct node *prev;
};
struct dl_list{
struct node *head;
struct node *tail;
int size;
};
struct node *create_empty_node();
struct node *create_node(void *);
void destroy_node(struct node *);
struct dl_list *create_empty_list();
struct node *search(struct dl_list *, void *, int (*comp)(void *,void *));
void insert_el_head(struct dl_list **, void *);
void insert_el_tail(struct dl_list **, void *);
void insert_node_head(struct dl_list **, struct node *);
void insert_node_tail(struct dl_list **, struct node *);
void insert_el_at(struct dl_list **, void *, int);
void insert_node_at(struct dl_list **, struct node *, int);
void delete_all(struct dl_list **, void *, int(*com)(void*,void*));
struct node *delete_el(struct dl_list **, void *, int (*comp)(void *, void *));
struct node *delete_head(struct dl_list **);
struct node *delete_tail(struct dl_list **);
struct node *delete_at(struct dl_list **, int);
struct node *delete_node(struct dl_list **, struct node *);
//uses delete_el, assuming comp == numcmp
struct node *delete_int(struct dl_list **, int);
void print_list(struct dl_list *);
int is_empty(struct dl_list *);
void clear_list(struct dl_list *);
void free_list(struct dl_list *);
int size(struct dl_list *);
//comparison functions
int numcmp(int *x, int *y);
#endifImplementation:
```
#include
#include
#include
#include
#include
#include "double_linked_list.h"
struct node *create_empty_node(){
struct node temp = (struct node ) malloc(sizeof(struct node));
temp->next = temp->prev = NULL;
temp->data = NULL;
return temp;
}
struct node create_node(void key){
void *
Solution
There are multiple issues with your code. The most obviously wrong is the use of double-pointers everywhere.
For example:
The double pointers result in the need to de-reference before use, as
in
functions should be written with single pointers:
Double pointers are necessary only when you need to change a pointer
(as opposed to what the pointer locates) in the calling function.
Your idea of data storage is odd. In your
to want to store just an integer value (as all of the data you store
in your tests is smaller or equal to a
you make a copy of the
This might be what you intended but all the same it looks wrong. Your
another
redundant. But more to the point if you wanted to store only data up
to the size of a pointer, you should just declare
It would be much simpler just to store an
This also makes it clearer to the caller the nature of the data that
can be stored. Your existing functions appear to offer the ability
to store data of any size. As I said, storing a single scalar type is much simpler.
With variable data types, the comparator functions need to know what data they are comparing, so your existing comparisons and any based upon a union as above are unlikely to be reliable without some extra information to identify the type of data (a discriminator for the union). If any given list contains only one data type, the problem is lessened, but you have no way to enforce that.
You have many functions taking a callback and you have many function
calls that cast a function parameter to be suitable for these functions:
These are ugly and make reading slower than it need be. It is better
to define a comparator type:
Note also the
The loop in your
You call
loop variable
unused until the third call of
examine each
the list according to whether
Note that I defined
the
I think that is enough for really, but a few minor points occur to me:
-
use
pointer parameter should define that parameter
-
don't cast the return from
can be harmful.
-
functions (and their prototypes) that take no parameters should
declare a
-
function point
For example:
void insert_node_head(struct dl_list **list, struct node *temp){
if(is_empty(*list)){
(*list)->head = (*list)->tail = temp;
++(*list)->size;
return;
}The double pointers result in the need to de-reference before use, as
in
(*list)->head. This is totally unnecessary. This and all otherfunctions should be written with single pointers:
void insert_node_head(struct dl_list *list, struct node *temp){
if(is_empty(list)){
list->head = list->tail = temp;
++list->size;
return;
}Double pointers are necessary only when you need to change a pointer
(as opposed to what the pointer locates) in the calling function.
Your idea of data storage is odd. In your
key parameters you seemto want to store just an integer value (as all of the data you store
in your tests is smaller or equal to a
void*). In create_nodeyou make a copy of the
key.struct node *create_node(void *key){
void *copy = malloc(sizeof(void *));
memcpy(copy, key, sizeof(key));This might be what you intended but all the same it looks wrong. Your
node structure contains a void *data field and you are allocatinganother
void* to hang off that, so at a minimum the allocation isredundant. But more to the point if you wanted to store only data up
to the size of a pointer, you should just declare
data differently.It would be much simpler just to store an
int or long and cast integer types to that, but if you really want to store varying data types, you might use a union (although unions are not widely used):union node_data {
int i;
long l;
char ch;
};
struct node{
union node_data data; // not a pointer
...
struct node *create_node(union node_data *data)
{
struct node *n = create_empty_node();
n->data = *data;This also makes it clearer to the caller the nature of the data that
can be stored. Your existing functions appear to offer the ability
to store data of any size. As I said, storing a single scalar type is much simpler.
With variable data types, the comparator functions need to know what data they are comparing, so your existing comparisons and any based upon a union as above are unlikely to be reliable without some extra information to identify the type of data (a discriminator for the union). If any given list contains only one data type, the problem is lessened, but you have no way to enforce that.
You have many functions taking a callback and you have many function
calls that cast a function parameter to be suitable for these functions:
struct node *search(struct dl_list *, void *, int (*comp)(void *,void *));
...
temp = search(head, (void *)&m, (int (*)(void *,void *))numcmp);These are ugly and make reading slower than it need be. It is better
to define a comparator type:
typedef int(*Comparator)(const void*,const void*);
...
struct node *search(struct dl_list *, void *, Comparator);
...
temp = search(head, (void *)&m, (Comparator) numcmp);Note also the
const in the Comparator definition.The loop in your
delete_all function is odd:struct node *cur;
struct node *temp;
for(cur = (*list)->head; cur != NULL; ){
//if first element is key, remove it
if((*comp)((*list)->head->data, key)){
cur = cur->next;
destroy_node(delete_head(list));
}
//if tail is key, remove it
if((*comp)((*list)->tail->data, key)){
cur = cur->next;
destroy_node(delete_tail(list));
}
//key found in cur->data
else if((*comp)(cur->data, key)){You call
comp three times, when one should suffice, and you have aloop variable
cur which traverses each entry in the list but isunused until the third call of
comp. It would be more normal toexamine each
cur using comp and then if there is a match, adjustthe list according to whether
cur is the head/tail or other:for (struct node *cur = list->head; cur != NULL; ...) {
if (comp(cur->data, key)){
/* do all the work in here !! */
if (cur is head) {
...
}
else if (cur is tail) {
...
}
else {
...
}
}
}Note that I defined
cur within the for-loop and assumed removal ofthe
list double-pointerI think that is enough for really, but a few minor points occur to me:
-
use
const everywhere you can. Functions that don't change apointer parameter should define that parameter
const. For example:void insert_el_head(struct dl_list *, const void *);
int size(const struct dl_list *);-
don't cast the return from
malloc. This is not needed in C andcan be harmful.
-
functions (and their prototypes) that take no parameters should
declare a
void parameter list.-
function point
Code Snippets
void insert_node_head(struct dl_list **list, struct node *temp){
if(is_empty(*list)){
(*list)->head = (*list)->tail = temp;
++(*list)->size;
return;
}void insert_node_head(struct dl_list *list, struct node *temp){
if(is_empty(list)){
list->head = list->tail = temp;
++list->size;
return;
}struct node *create_node(void *key){
void *copy = malloc(sizeof(void *));
memcpy(copy, key, sizeof(key));union node_data {
int i;
long l;
char ch;
};
struct node{
union node_data data; // not a pointer
...
struct node *create_node(union node_data *data)
{
struct node *n = create_empty_node();
n->data = *data;struct node *search(struct dl_list *, void *, int (*comp)(void *,void *));
...
temp = search(head, (void *)&m, (int (*)(void *,void *))numcmp);Context
StackExchange Code Review Q#38010, answer score: 10
Revisions (0)
No revisions yet.