1
\$\begingroup\$

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!

\$\endgroup\$

closed as off-topic by vnp, yuri, pacmaninbw, t3chb0t, Graipher May 12 at 17:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – vnp, yuri, pacmaninbw, t3chb0t, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Relevant: Titling your question in the Help Center. \$\endgroup\$ – AlexV May 7 at 19:48
  • \$\begingroup\$ @yuri I don't really care about this specific classes implementation, rather I'm trying to understand why/when to use keywords etc. The class itself is irrelevant and instead just being used as an example. \$\endgroup\$ – Hex May 7 at 19:48
  • \$\begingroup\$ @AlexV What would you title it if the question isn't about the class code, and instead about the c++ features I'm asking about? Because I could put any class code in the example, the questions would remain the same. \$\endgroup\$ – Hex May 7 at 19:50
  • 3
    \$\begingroup\$ "General purpose questions" are IMHO not suited for Code Review. As the Help Center puts it: "In order to give good advice, we need to see real, concrete code, and understand the context in which the code is used. Generic code [...] leaves too much to the imagination." (source). \$\endgroup\$ – AlexV May 7 at 19:55
  • 8
    \$\begingroup\$ Since you are asking to take advantage of language features, please specify what version of C++ you are targeting. \$\endgroup\$ – 200_success May 7 at 21:10
6
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ It is highly questionable to suggest naming conventions that arecontradictory to standard practices demonstrated by the standard (e.g empty vs is_empty) \$\endgroup\$ – miscco May 8 at 17:27
  • \$\begingroup\$ Thanks for taking the time to go in depth on everything, I guess I should clarify why I hadn't put anything in place to support different types of T, currently the code was only being used by another data-structure that only uses unsigned ints with it, as far as the get and put, those are also both only called by the data structure so i hadn't really bothered to make it very client facing friendly. That said, thanks for catching the empty bug. Out of curiosity, would adding all the extra stuff to support all the other types, add much overhead? I'm trying to keep this as fast as possible. \$\endgroup\$ – Hex May 8 at 19:20
  • \$\begingroup\$ also what do you mean with "When you add an element in put you have to placement new the element: new &buffer[head * sizeof(T)] T(std::move(item));." I'm not sure how that code is supposed to work? \$\endgroup\$ – Hex May 8 at 19:39
  • \$\begingroup\$ When you use new it normally allocates dynamic memory, constructs an object of given type with given arguments there and returns a pointer to that object. But you can optionally provide memory instead. (I messed up the syntax and forgot the parenthesis around the address). That way it constructs an object in the given memory without needing dynamic memory allocation. It's only needed if you have need a memory buffer that sometimes contains objects and sometimes doesn't, which is the case here if you decide to support creating and destroying objects as needed. \$\endgroup\$ – nwp May 9 at 7:30
  • \$\begingroup\$ As for the performance of placement new, the constructor and destructor for unsigned int don't do anything at runtime, so there should be no difference. Technically you are not allowed to access objects that don't exist, so you can't just cast the address of buffer to a T * and access the T that isn't there. That also applies to unsigned ints. \$\endgroup\$ – nwp May 9 at 7:31

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