Take the 2-minute tour ×
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
add comment

2 Answers

up vote 4 down vote accepted

Looks nice, is very readable and is 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
add comment
  • I agree with @User1 about RB_pop() and would like to add on to it:

    If you want to prevent the function from returning false as an unexpected return value, you can make the function void and have a second parameter for data that will update the argument passed into it. Also, by making it void, you can return early if the first condition is false.

    void RB_pop(ring_buffer* rb, int8_t& data)
    {
        if (rb == NULL || rb->buffer == NULL)
            return;
    
        // update data parameter...
    }
    
  • RB_free() looks a little too simple, so free() can just be called inline. Both function names also read the same, so you don't need an extra one to specify the meaning.

share|improve this answer
add comment

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.