Move ring buffer out of toxcore/util into toxav. #163
Conversation
iphydf
force-pushed
the
ringbuffer
branch
3 times, most recently
from
2a77f16
to
d864c50
Compare
| // XXX: Hack because toxav doesn't really expose ring_buffer, but this av test | ||
| // uses it. | ||
| #define RingBuffer TestRingBuffer | ||
| #define rb_full test_rb_full |
There was a problem hiding this comment.
Most of these aren't actually used in this test. They should be removed to make the code easier to read
There was a problem hiding this comment.
They aren't used, but if linking statically, not renaming them will cause multiple definition errors. We need to rename all of them.
There was a problem hiding this comment.
Maybe mention the static linking issue in the comment too.
There was a problem hiding this comment.
What's the need for ring_buffer compilation here?
| @@ -0,0 +1,18 @@ | |||
| #ifndef RING_BUFFER_H | |||
There was a problem hiding this comment.
This header needs a comment as to the point of RingBuffer (and maybe a rename!),
FYI ring buffer stores AV packets received so ToxAV can calculate the mean on how many packets are lost/received.
There was a problem hiding this comment.
I don't know the point of RingBuffer. This PR just moves it out of toxcore where it doesn't belong. @mannol can add documentation.
There was a problem hiding this comment.
@GrayHatter It's literally a name of the data type. I don't see a point in renaming it for this special case.
There was a problem hiding this comment.
OH... I've been reading it as ring, as in ringing call... too much AV on the brain
| // XXX: Hack because toxav doesn't really expose ring_buffer, but this av test | ||
| // uses it. | ||
| #define RingBuffer TestRingBuffer | ||
| #define rb_full test_rb_full |
There was a problem hiding this comment.
Maybe mention the static linking issue in the comment too.
| @@ -0,0 +1,18 @@ | |||
| #ifndef RING_BUFFER_H | |||
There was a problem hiding this comment.
@GrayHatter It's literally a name of the data type. I don't see a point in renaming it for this special case.
| // XXX: Hack because toxav doesn't really expose ring_buffer, but this av test | ||
| // uses it. | ||
| #define RingBuffer TestRingBuffer | ||
| #define rb_full test_rb_full |
There was a problem hiding this comment.
What's the need for ring_buffer compilation here?
iphydf
force-pushed
the
ringbuffer
branch
3 times, most recently
from
25c2694
to
311fbce
Compare
|
Since I can't reply to the review thread (@mannol do you still like github reviews better than reviewable?):
|
|
I do. We should just avoid force pushing; at least until the review is complete. |
|
The symbols are not part of the ABI, and since #117, toxav does not export those symbols in the .so file anymore (on linux). |
Toxcore itself doesn't use this data structure. Only toxav does, so now toxav owns the code for it.
Toxcore itself doesn't use this data structure. Only toxav does, so now
toxav owns the code for it.