Simple ring/circular buffer C++ class [closed]

Question

I've got this simple ring/circular buffer class:

template<class T, size_t sz>
class CircularBuffer {

    std::array<T, sz> buffer;
    size_t head;
    size_t tail;
    bool isFull;

public:
    CircularBuffer() :
        head{0},
        tail{0},
        isFull{false} {
    }

    void put(T item) {
        buffer[head] = item;
        head = (head + 1) % sz;
        if (isFull) {
            tail = (tail + 1) % sz;
        }
        isFull = head == tail;
    }

    T get() {
        auto result = buffer[tail];
        tail = (tail + 1) % sz;
        isFull = false;
        return result;
    }

    bool empty() const {
        return tail == head;
    }

    size_t capacity() const {
        return sz;
    }

    size_t size() const {
        if (isFull)
            return sz;

        if (head >= tail)
            return head - tail;

        return sz + head - tail;
    }
};

And I was looking for clarification on a few things, to take advantage of C++ features.

First, the new constexpr keyword, what here, if anything should I apply it to? (I'm assuming the size_t size() const member function could use it? Anything else?)

Second, all of these member functions are quite small, should they be inlined?

Third, in the T get() member function, I do auto result = buffer[tail];, should I use auto& instead, or any other versions? (or even just T/T&?) Should that be a const as it's not modified within the function, and only potentially modified once a copy is returned via the functions return parameter.

Any other feedback is welcome!

Answer 1

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 Ts 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 Ts 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.