Preserve circular buffer item semantics#93
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the circular buffer implementation so that operations consistently treat the underlying storage as item-sized records (not bytes), improving correctness for non-item_size == 1 buffers while keeping FIFO behavior unchanged.
Changes:
- Added input validation in
circular_buffer_push/circular_buffer_getto ignore invalid buffer/item pointers and zero-sized configurations. - Fixed indexed removal (
circular_buffer_pop) to shift whole items to preserve item semantics. - Fixed iteration to pass item-aligned pointers to callbacks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t src = (cb->tail + i - 1) % cb->length; | ||
| memcpy(cb->buffer + dst * cb->item_size, cb->buffer + src * cb->item_size, cb->item_size); | ||
| --i; | ||
| } |
There was a problem hiding this comment.
What is this supposed to be doing?
There was a problem hiding this comment.
circular_buffer_pop() accepts an index. previously it returned the item at that index while always removing the item at the tail. This loop shifts the preceding items forward so advancing the tail removes the requested item instead. For example, popping index 1 from [A, B, C] returns B and leaves [A, C].
Worth noting that there's currently no callers of this - but if it were to be called, it would not do what the docs said, thus leaving a hard to find bug.
Alternatively, deleting this would probably work just fine. It’s a footgun if left as-is for someone to use later.
There was a problem hiding this comment.
Ok, I didn't even notice the i there when reading the PR - the pop method was written for completeness and I honestly don't know how it came around with index as an argument (maybe as an analogy to the get method...). For a circular buffer queue implementation the pop should just remove from the back, the index and shifting the array doesn't really make sense. So if you want just remove the index and pop from the tail, but the shifting of elements is not desirable.
Prime unit test material for sure.
There was a problem hiding this comment.
yep, I caught that when doing the test suite. I'll update this to work the way you suggested.
59bfc5f to
ab1fb35
Compare
Make circular-buffer operations consistently use item-sized records.
Indexed removal now shifts whole items, iteration callbacks receive item-aligned pointers, and invalid buffer or item pointers are ignored. Valid FIFO behavior is unchanged.
Updated based on feedback
Removed the guads as requested. I redid the PR in two commits because they fix separate circular-buffer behaviors.
The pop fix makes
circular_buffer_pop(cb, i, ...)match its documented indexed-removal semantics. Previously it returned item i, but always advanced tail, effectively removing item 0. The new loop shifts the items before i forward by one slot, then advances tail, removing item i while preserving the order of everything else. For example, popping index 1 from [A, B, C] returns B and leaves [A, C]. That function is currently unused so deleting it is also a reasonable solution.The iteration fix accounts for buffer being byte-addressed while i is an item index. &buffer[i] advanced by one byte; buffer + i * item_size advances by one complete item. This matters for every multi-byte record, including recorder samples.
The iteration fix fixes a demonstrable bug in recording:
Sample is 32 bytes: a timestamp, flags, and 13 recorded values. The recorder configures the buffer with
item_size = sizeof(Sample)in data_recorder.c:69, then passes iteration pointers to a callback that casts them toSample *at data_recorder.c:133.