ToxAV's rb_write function is written in a strange way

[zoff99]

c-toxcore/toxav/ring_buffer.c

Lines 60 to 61 in 1a54be0

	if (b->end == b->start) {
	b->start = (b->start + 1) % b->size;

there is something i don't understand.
if this is ever reached should not also rc be set to start element. since now the start element is also pushed out?

me loves me some code without documentation ...

[zoff99]

i think this should look like this (to make it clear):

void *rb_write(RingBuffer *b, void *p)
{
    void *rc = nullptr;

    if ((b->end + 1) % b->size == b->start) { /* buffer full ? */
        rc = b->data[b->start]; // return oldest element
        b->start = (b->start + 1) % b->size; // insert an empty element (this distinguish from empty state)
    }

    b->data[b->end] = p;
    b->end = (b->end + 1) % b->size;

    return rc;
}

[sudden6]

if this is ever reached should not also rc be set to start element. since now the start element is also pushed out?

Seems to happen a few lines above

c-toxcore/toxav/ring_buffer.c

Line 54 in 1a54be0

rc = b->data[b->start];

But I agree with you, the variant you provide is more readable(!). Just the comment insert an empty element (this distinguish from empty state) is wrong. There's already something in the buffer at that position, we just let start point to the new "first" element.

I have to say this is an interesting ringbuffer implementation, I would not have expected the inserted element in the buffer and the "first" element returned when the buffer is full. Needs some docs.

[zoff99]

it allocates (size + 1) in new. so there seems to be always an empty element there. or something?
when full still writes the new element inside, but pushes the oldest out. caller needs to free it.

[sudden6]

it allocates (size + 1) in new. so there seems to be always an empty element there. or something?

Seems so, also strange IMO, because normally you don't need an empty element. AFAICT when the buffer is full once, there's also no empty element anymore.

[iphydf]

It's still strange, but we've added a test for the behaviour, so this ticket is done.