-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core/cyclicbuffer] : cherry pick from '6ee3271693a84897b0208f2af954d…71054b1eed4'. #1558
Conversation
…71054b1eed4'. Include the last element in writing. It is unintuitive that the anmount of data is always 1 less than the (maximum) size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that in theory Write() and Read() should be symmetrical, and it is in fact strange that the ASSERT in Read() is less equal
meanwhile here in Write it is just less
. So, in theory, it should probably be less equal
.
But could you test if perhaps some other changes are necessary because of changing this ASSERT?
You could use the tests for each overflow corner case I made in this commit (ff3c391), and modify them so that the buffers one byte bigger.
Just ran the these modified tests. One fails. OffsetCutPushDataShouldFlushOldDataIfDoesNotFitOffsetCut with uint8_t fullBufferSimulation[DATA_SIZE - sizeof(uint16_t)] fails with ERROR_WRITE_ERROR. |
@sebaszm should we leave it for now as it is, and have a look into it in the future? Since changing the ASSERT clearly means we have to make some more changes to make both Read() and Write() symmetrical |
Closing PR as it has been decided to accept the mismatch. |
Include the last element in writing. It is unintuitive that the anmount of data is always 1 less than the (maximum) size.