Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to get into C stuff, and I thought it would be a good idea to try and implement a circular buffer.

I have defined my struct like this:

typedef struct
{
     int8_t* buffer;
     int8_t* buffer_end;
     int8_t* data_start;
     int8_t* data_end;
     int64_t count;
     int64_t size;
 } ring_buffer;

And the functions:

void RB_init(ring_buffer* rb, int64_t size)
{
    rb->buffer = malloc(sizeof(int8_t) * size);
    rb->buffer_end = rb->buffer + size;
    rb->size = size;
    rb->data_start = rb->buffer;
    rb->data_end = rb->buffer;
    rb->count = 0;
}

void RB_free(ring_buffer* rb)
{
    free(rb->buffer);
}

bool RB_push(ring_buffer* rb, int8_t data)
{
    if (rb == NULL || rb->buffer == NULL)
        return false;

    *rb->data_end = data;
    rb->data_end++;
    if (rb->data_end == rb->buffer_end)
        rb->data_end = rb->buffer;

    if (RB_full(rb)) {
        if ((rb->data_start + 1) == rb->buffer_end)
            rb->data_start = rb->buffer;
        else
            rb->data_start++;
    } else {
        rb->count++;
    }

    return true;
}

int8_t RB_pop(ring_buffer* rb)
{
    if (rb == NULL || rb->buffer == NULL)
        return false;

    int8_t data = *rb->data_start;
    rb->data_start++;
    if (rb->data_start == rb->buffer_end)
        rb->data_start = rb->buffer;
    rb->count--;

    return data;
}

bool RB_full(ring_buffer* rb)
{
    return rb->count == rb->size;
}

I did some testing and it seems to work well. Can you suggest some improvements ?

share|improve this question
RB_pop does not check for an empty buffer and has no way of indicating that the buffer is empty. – William Morris Oct 12 '12 at 14:24
I would consider it better style to use size_t instead of int64_t for all sizes and counts of in-memory objects. – Seg Fault Oct 27 '12 at 7:11

1 Answer

up vote 1 down vote accepted

Looks nice, very readable and probably fast.

Sometimes ring buffer wrap is implemented by using the following kind of remainder stuff and offsets:

end_offset = (end_offset + 1) % size;

But I like your way to do it without offsets and without division.

Some minor findings:

1)

NULL pointer checks in RB_pop prevents segfaults, but caller will get return value zero. So caller won't know is zero an error or success result.

int8_t RB_pop(ring_buffer* rb)
{
    if (rb == NULL || rb->buffer == NULL)
        return false;

2)

RB_pop and RB_push both do the check: rb == NULL. Maybe other functions should do it too.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.