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

Queue implementation in C

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

Problem

I was reading about opaque pointers and decided to try it with queues. Please review my code and let me know if there are any errors and how to improve code quality and performance.

It uses lines to enqueue and dequeue. All lines have fixed sizes and dequeue/enqueue never share a line. New lines are added only when all lines are full, not counting the dequeue line. The lines are stored in a linked list.

queue.h

#ifndef QUEUE_H
#define QUEUE_H

#include 
#include 

#define QU_SUCCESS 0
#define QU_ERROR 1

typedef struct Queue Queue;

Queue *qu_allocate_custom(size_t lines, size_t capacity);
Queue *qu_allocate(void);
void qu_free(Queue *queue);
int qu_enqueue(Queue *queue, void *content);
void *qu_dequeue(Queue *queue);
size_t qu_get_count(Queue *queue);
void qu_shrink_to_fit(Queue *queue);

#endif


queue.c

```
#include "queue.h"

#define QU_DEFAULT_LINES 4
#define QU_DEFAULT_LINE_CAPACITY 50

//Data structures
/*
The line has a fixed size. It contains a link to the next and can hold void *.
I'm using double pointers because I expect it to act like an array of void *.
So, when I do position++ it will move sizeof(void *) and point to the next
stored address. Also the end of the line, if it's not full, can be delimited by
simply doing *position = NULL.
*/
typedef struct Line Line;
struct Line {
void **start;
void **position;
void **end;
Line *next;
};

/*
The queue structure just stores meta info and points to lines. The member index
is there because I thought it would make the the other functions simpler since
they won't have to deal with special cases. They just treat it as if it were a
regular line. And that's ok because most functions take the line that comes before
the one they are operating on as an argument so they don't have to walk through
the list to find that node in order to update the pointer to the next;

The queue stores only elements of sizeof(void *), whatever is received by enqueue
will be sent back by dequeue, so it can st

Solution

Your header looks fine - opaque pointers are as simple as that. Don't forget
the const on function parameters (pointers) where possible.

Hmmm, a Line structure with double-pointers. That looks suspect. Double
pointers are usually unnecessary. We'll see what becomes of these later...
It would have been kind to the reader to give a line or two of comment to
describe the queue and its components and the purpose of these double
pointers.

The use of previous as a parameter name is unsatisfying, somehow
confusing. In free_line_after I'm expecting it to free the line after 'this'
one or the 'current' one, so previous jars a bit. I would avoid 'this'
though and call it just l.

The start parameter to free_all_lines is misnamed as there is a start
field in Line. The two are unrelated (I think) and should have different
names.

In push_line_after you are allocating

Line *new_line = malloc(sizeof(Line) + capacity * sizeof(void **));


So this makes me think that your Line structure is intended to hold the line
data beyond the end of the structure, as in:

struct Line {
    ...
    Line *next;
    char data[];
};


You instantiate Line in Queue for some reason (I imagine from your comment
that you just need a pointer to a Line but I haven't checked) so you'd have
to have char data[1] to keep that. But this is the normal way to extend a
structure with variable data. If you do this, you don't need the double
pointers but instead just keep indices.

Maybe I have misinterpreted your intentions, so I will stop there. I haven't
read through the rest (as I say double pointers are normally unnecessary and
make reading code difficult).

EDIT I don't have time to look in detail at the code, but the explanations you
added certainly help. I do think the mechanism is rather odd - having two queue mechanisms in one. Your tests only test one of the mechanisms with the code in
conditional in qu_enqueue:

if(queue->enqueue->end == queue->enqueue->position){
    ....
}


never entered. Setting QU_DEFAULT_LINE_CAPACITY to something small rectifies this and the code seems to work.

Code Snippets

Line *new_line = malloc(sizeof(Line) + capacity * sizeof(void **));
struct Line {
    ...
    Line *next;
    char data[];
};
if(queue->enqueue->end == queue->enqueue->position){
    ....
}

Context

StackExchange Code Review Q#38556, answer score: 3

Revisions (0)

No revisions yet.