Interface
Naming
Functions returning a bool
should be phrased as a question. empty
should be is_empty
instead. Yes, the standard library does it wrong too, leading to confusion like "I used vector.empty();
, but it didn't empty my vector. Why?"
get
should be pop
or pop_get
. Getters are not supposed to change the object.
Note that it is impossible to write get
with the strong exception guarantee, which is the reason why std::vector::pop_back
returns void
instead of the element.
constexpr
Currently you can mark all your functions constexpr
. Sometimes it is possible to evaluate the result of your CircularBuffer
at compile time. That probably rarely comes up, but there is no good reason not to do it (yet).
Generality
Type restrictions
There are limits for what T
s I can use your CircularBuffer
with. T
must be copyable and default constructible. That means I cannot use a struct Foo{ Foo(int); };
or a std::unique_ptr<int>
. Arguably those should be allowed.
Move-Only
Supporting move-only types is possible by using std::move
in the appropriate spots, mainly buffer[head] = std::move(item);
and auto result = std::move(buffer[tail]);
. Just try to use a CircularBuffer<std::unique_ptr<int>>
and the compiler will tell you about each spot.
Non-Default-Constructible
To be able to use CircularBuffer<Foo>
you would need to delay constructing objects until the user uses put
. You can achieve that by changing std::array<T, sz> buffer;
to alignas(alignof(T)) std::array<char, sz * sizeof(T)> buffer;
. That way no T
s are default constructed. When you add an element in put
you have to placement new
the element: new (&buffer[head * sizeof(T)]) T(std::move(item));
. get
then has to call std::destroy_at(reinterpret_cast<T*>(&buffer[tail * sizeof(T)]));
(or just call the destructor). This makes things more complicated and also reinterpret_cast
and new
are not constexpr
.
Brick Types
Some types like std::mutex
cannot be copied or moved, but you could still support them. To do that, offer an emplace
function similar to std::vector::emplace_back
that constructs the T
in place from a given list of arguments.
get
Return Type
Returning a T
by value seems reasonable. You are taking out the element. Returning a T &
instead seems dangerous, because usage of the buffer will eventually change the value you got. Maybe add 2 peek
functions instead that return a reference to the current object without removing it. One of the functions would be T &peek()
and the other const T &peek() const
.
Bugs
empty
When Full
CircularBuffer<int, 3> b;
b.put(1);
b.put(2);
b.put(3);
std::cout << std::boolalpha << b.empty();
That should really not print true
.
Over- and Underflow
If I put
more items into the buffer than it has space it silently overwrites objects. If I try to get
items without putting items in, it simply returns uninitialized objects which is undefined behavior for builtins. This is my fault for using your container incorrectly, but you could be nice and add an assert
so that I can find my bug easier.
inline
Your functions are already implicitly marked inline
which changes the linkage and has nothing to do with inlining. Whether inlining is the right choice is a complicated case-by-case question that you should leave to your compiler. Only use inline
to mean "I want internal linkage", which you can also do for variables since C++17.