I am learning circular / cyclic buffer using this link. Here is the code that I have written so far:
#include <iostream>
#include <memory>
#include <cstddef>
class CircularBuffer
{
std::unique_ptr<int[]> u_ptr;
std::size_t head;
std::size_t tail;
const std::size_t capacity;
bool full_v;
public:
CircularBuffer(std::size_t);
void reset();
bool full() const;
bool empty() const;
std::size_t size() const;
void write(int);
int read();
};
// Constructor
// Set the capacity via initializer list because it is const
CircularBuffer::CircularBuffer(std::size_t space):capacity{space}
{
u_ptr = std::unique_ptr<int[]>(new int[space]);
head = 0;
tail = 0;
full_v = false;
}
// Reset the Buffer
void CircularBuffer::reset()
{
head = 0;
tail = 0;
full_v = false;
u_ptr[head] = int{};
}
// Check if buffer is full
bool CircularBuffer::full() const
{
return full_v;
}
// Check if buffer is empty
bool CircularBuffer::empty() const
{
return (!full_v && head == tail);
}
// Return the size of the buffer
std::size_t CircularBuffer::size() const
{
if(full_v)
{
return capacity;
}
if(head >= tail)
{
return head - tail;
}
else
{
return capacity - (tail - head);
}
}
// Write values into the buffer
void CircularBuffer::write(int data)
{
u_ptr[head] = data;
head = (head + 1) % capacity;
if(full_v)
{
tail = (tail + 1) % capacity;
}
full_v = (head == tail);
}
// Read from buffer
int CircularBuffer::read()
{
if(this -> empty())
{
return int{};
}
int ret_val = u_ptr[tail];
full_v = false;
tail = (tail + 1) % capacity;
return ret_val;
}
int main()
{
CircularBuffer cb{10};
std::cout << "Empty: " << cb.empty() << "\n";
for(int i = 0; i < 10; i++)
{
cb.write(i);
}
std::cout << "Full: " << cb.full() << "\n";
std::cout << "Read: " << cb.read() << "\n";
std::cout << "Full: " << cb.full() << "\n";
std::cout << "Empty: " << cb.empty() << "\n";
std::cout << "Size: " << cb.size() << "\n";
cb.write(35);
std::cout << "Size: " << cb.size() << "\n";
std::cout << "Read: " << cb.read() << "\n";
std::cout << "Size: " << cb.size() << "\n";
cb.reset();
std::cout << "Size: " << cb.size() << "\n";
return 0;
}
Is the implementation correct? What are the shortcomings that can be fixed?
I see that the original reference uses a
mutex
object and uses lock. Is such an approach used with all data structures? If not, is there a reason why that has been used here?
2 Answers 2
Your ringbuffer has a compile time size, so it would be appropriate to make it a template class rather than simply passing it as an argument to the constructor. You never plan to change it anyway?
template<size_t bufSize>
class CircularBuffer
That leads to the next point. You use a std::unique_ptr<int[]>
There is something in C++ that is much better than this std::array
. With the size of the buffer a compile time template argument of the class you can easily use it
template<size_t bufSize>
class CircularBuffer {
std::array<int, bufSize> buf;
std::size_t head;
std::size_t tail;
bool full_v;
...
};
Note that this increases the size of the class object considerably, as the std::array
is directly inlined in the class.
The other members of this class are always the same after construction so you should use static member initialization:
template<size_t bufSize>
class CircularBuffer {
std::array<int, bufSize> buf;
std::size_t head{ 0 };
std::size_t tail{ 0 };
bool full_v{ false };
...
};
Note that this completely removes the need to define a constructor at all. The compiler generated default constructor will do just fine.
Resetting the buffer should not change the data stored in it as it is not read anyway so just leave it alone
void CircularBuffer::reset()
{
head = 0;
tail = 0;
full_v = false;
}
Can tail ever be larger than head? If not why are you checking it.
-
\$\begingroup\$ Thank You for the review. Imagine a situation where buffer is full and both head and tail are in the middle of the buffer. Then reading happens. The tail keeps increasing while head remains in its position. So, in this case, tail can be ahead of head. \$\endgroup\$skr– skr2018年09月30日 19:03:59 +00:00Commented Sep 30, 2018 at 19:03
Correctness and design
I will skip all points miscco
already raised and go directly to void write
and int read
.
- Your
write
overwrites old values when the buffer is full. - Your
read
returns zero if the buffer is empty.
Although understandable, it should at least be documented:
/// Describe the class, what is the purpose, what is important
template<class T, size_t N> // see std::array
class CircularBuffer {
...
/// Write values into the buffer, overwrite old values when the buffer is full
void write(const T& data) {
or maybe:
/// Write values into the buffer
///\return true if written, false if buffer was full
bool write(const T& data) {
You can also notice that I made it a template, just like std::array
- both the type and the capacity are now template parameters. The standard way of passing arguments to methods like write
is to use const T&
, but it has some drawbacks, so, if it trully is for embedded systems, then simple T
is an option as well (knowing you are going to use it with integers and such). But document it (and/or add other template parameters with some defaults).
Synchronization (Locking)
If you are going to use it with multiple threads (or main loop vs. interrupt) then some form of synchronization is indeed needed, because there are possible race-conditions otherwise, like one thread being inside write
, just overwriting the oldest slot, while another thread can be inside read
getting this newest element, while it should get the oldest, but tail
was not yet updated by the first thread.
First question to ask is: How many threads / concurrent readers and writers (producers and consumers)?
Generic solution could use mutex
but there can be better solutions for single producer single consumer (e.g. main loop vs. interrupt - USART and similar communication synchronization). One possibility is to use separate (atomic) write and read counters (like head and tail but you always only increase the counters and substract them to get number of elements inside the buffer - but be careful with order of operations, it is not trivial).