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

Doubly linked list with no apparent memory leaks

Submitted by: @import:stackexchange-codereview··
0
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:

#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);
#endif


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 *

Solution

There are multiple issues with your code. The most obviously wrong is the use of double-pointers everywhere.

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 other
functions 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 seem
to 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_node
you 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 allocating
another void* to hang off that, so at a minimum the allocation is
redundant. 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 a
loop variable cur which traverses each entry in the list but is
unused until the third call of comp. It would be more normal to
examine each cur using comp and then if there is a match, adjust
the 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 of
the list double-pointer

I think that is enough for really, but a few minor points occur to me:

-
use const everywhere you can. Functions that don't change a
pointer 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 and
can 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.