patterncMinor
Queue implementation in C
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
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
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);
#endifqueue.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
Hmmm, a
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
confusing. In
one or the 'current' one, so
though and call it just
The
field in
names.
In
So this makes me think that your Line structure is intended to hold the line
data beyond the end of the structure, as in:
You instantiate
that you just need a pointer to a
to have
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
never entered. Setting
the
const on function parameters (pointers) where possible.Hmmm, a
Line structure with double-pointers. That looks suspect. Doublepointers 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, somehowconfusing. 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 startfield in
Line. The two are unrelated (I think) and should have differentnames.
In
push_line_after you are allocatingLine *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 commentthat you just need a pointer to a
Line but I haven't checked) so you'd haveto have
char data[1] to keep that. But this is the normal way to extend astructure 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.