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

2 Answers 2

up vote 6 down vote accepted

This looks nice. It 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 of doing it without offsets and division.

Some minor findings:

  1. NULL pointer checks in RB_pop prevents segfaults, but the caller will get a return value of zero. So the caller won't know if zero is an error or a 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

I agree with @User1 about RB_pop() and would like to add on to it:

In order to prevent the function from returning an unexpected return value of false, you should make the function void and have a second parameter data. This will also allow you to return early if the first conditional statement is false.

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

    // update data parameter...
}
share|improve this answer
1  
The point of RB_free is encapsulation, and replacing it with plain free would defeat that. He likely does not intend for consumers of the data type to operate on the struct directly but rather use the interface exposed via the functions. That allows him to alter how the internals work without worry about breaking consuming code. If he makes the end user call free explicitly, then suddenly this is ruined. What if he were to add a second buffer? Suddenly every free() needs a new one. Using a function to handle it means just changing one place, encapsulating that functionality. –  Corbin May 30 at 5:23
    
@Corbin: Ah, okay. Thanks. I wasn't aware that that was the intent here, also considering that this is C. I'll remove it. –  Jamal May 30 at 5:32

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.