Enhancing a non-multithreaded Circular buffer

Question

Looking to get some feedback on a simple non-multithreaded circular buffer.

Ideally, I'd want to have a logic to prevent the data from getting overwritten but couldn't find a reasonable way. For instance, if the buffer size is 3 and Write is invoked, it shouldn't proceed until the buffer is no longer full.

class Fifo
{
    public:
    void Write(uint8_t data)
    {   
        mBuffer[mWriteIdx++] = data;
        mWriteIdx %= BUFFER_SIZE;
        mIsFull = mWriteIdx == mReadIdx;
    }
    
    uint8_t Read() 
    {
        if (IsEmpty())
        {
            return 0; // considering 0 is an invalid data
        }
        
        uint8_t value = mBuffer[mReadIdx];
        
        mBuffer[mReadIdx++] = 0;   // null out the index
        mReadIdx %= BUFFER_SIZE;
        mIsFull = false;
        
        return value;
    }
    
    
    private:
    
    bool IsEmpty() const
    {
        return !mIsFull && mReadIdx == mWriteIdx;
    }
    
    // Member variables
    int mReadIdx = 0;
    int mWriteIdx = 0;
    
    static constexpr int BUFFER_SIZE = 4;
    uint8_t mBuffer[BUFFER_SIZE] = {0};
    bool mIsFull = false;
};

Here's a sample program with outputs

Answer 1

Advice I: IsEmpty

bool IsEmpty() const
    {
        return !mIsFull && mReadIdx == mWriteIdx;
    }

Why not deploy a field

size_t size = 0;

counting the number of elements in the buffer? That way, your IsEmpty becomes much simpler:

bool IsEmpty() const 
{
    return size == 0;
}

Advice II: Add IsFull How about this?

bool IsFull() const {
    return size == BUFFER_SIZE;
}

Advice III: Throwing appropriate exceptions

I would throw an std::underflow_error when trying to read from an empty buffer, and I would throw an std::overflow_error when trying to write to a full buffer.

Summa summarum

All in all, I thought about this implementation:

#include <cstdlib>
#include <cstdint>
#include <stdexcept>
#include <iostream>

class Fifo
{
public:
    void Write(uint8_t data)
    {
        if (IsFull())
        {
            throw std::overflow_error("The buffer is full.");
        }

        mBuffer[(headIndex + size++) % BUFFER_SIZE] = data;
    }

    uint8_t Read()
    {
        if (IsEmpty())
        {
            throw std::underflow_error("The buffer is empty.");
        }

        uint8_t value = mBuffer[headIndex++];
        headIndex %= BUFFER_SIZE;
        size--;
        return value;
    }


private:

    bool IsEmpty() const
    {
        return size == 0;
    }

    bool IsFull() const {
        return size == BUFFER_SIZE;
    }

    // Member variables
    size_t headIndex = 0;

    static constexpr int BUFFER_SIZE = 4;
    uint8_t mBuffer[BUFFER_SIZE] = { 0 };
    size_t size = 0;
};

int main()
{
    Fifo fifo;

    for (int i = 0; i < 4; i++)
    {
        fifo.Write(i + 2);
    }

    for (int i = 0; i < 4; i++)
    {
        std::cout << unsigned(fifo.Read()) << "\n";
    }

    fifo.Write(10);
    return 0;
}

Hope that helps.

Answer 2

Use of the type uint8_t suggests that you're either including the C-compatibility header <stdint.h> or have a using at global scope.

Don't do either of those things. Include the C++ header <cstdint> and use the type it declares: std::uint8_t.

You might want to have

using value_type = std::uint8_t;

within the class. That will be helpful if you make it a template in future.

---

I would have liked to see a full set of unit tests for this class - it's perfectly suited to such testing.