12
\$\begingroup\$

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 ?

\$\endgroup\$
3
  • \$\begingroup\$ RB_pop does not check for an empty buffer and has no way of indicating that the buffer is empty. \$\endgroup\$ – William Morris Oct 12 '12 at 14:24
  • \$\begingroup\$ I would consider it better style to use size_t instead of int64_t for all sizes and counts of in-memory objects. \$\endgroup\$ – Seg Fault Oct 27 '12 at 7:11
  • \$\begingroup\$ Nice example - with the recommend (small) changes provide by SKi and Jamel and some minor tweeking, I used to this create a ring buffer to manage bytes being written to flash. Nice and easy to read. \$\endgroup\$ – PhillyNJ May 30 '18 at 12:48
7
\$\begingroup\$

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.

\$\endgroup\$
7
\$\begingroup\$

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...
}
\$\endgroup\$
0
0
\$\begingroup\$

you didn't check count in pop function. if buffer is empty you will return garbage and also count will now be negative.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ We prefer answers that make new observations; your answer merely duplicates parts of existing answers, and doesn't add any value here. Please find something else within the code that can be improved! \$\endgroup\$ – Toby Speight Nov 28 '18 at 12:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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