Declare BitReader::refill unsafe and document safety prerequisites #397
Conversation
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| @@ -1862,15 +1862,31 @@ fn inflate_fast_help_impl<const FEATURES: usize>(state: &mut State, _start: usiz | |||
| let mut bad = None; | |||
|
|
|||
| if bit_reader.bits_in_buffer() < 10 { | |||
| bit_reader.refill(); | |||
| if bit_reader.bytes_remaining() >= 8 { | |||
There was a problem hiding this comment.
I added this check to make sure the safety precondition was met. But if there's any way to know for sure that buffer.bytes_remaining() is guaranteed to be >= 8 when this function is called, I can document that with a comment in place of the if-statement here.
| @@ -1909,7 +1925,8 @@ fn inflate_fast_help_impl<const FEATURES: usize>(state: &mut State, _start: usiz | |||
| // we have two fast-path loads: 10+10 + 15+5 = 40, | |||
| // but we may need to refill here in the worst case | |||
| if bit_reader.bits_in_buffer() < MAX_BITS + MAX_DIST_EXTRA_BITS { | |||
| bit_reader.refill(); | |||
| // Safety: TODO | |||
There was a problem hiding this comment.
I'm still trying to determine how it's guaranteed to be safe to call bit_reader.refill() here. I imagine there's a limit on how many times this inner loop can be executed, and that's part of the proof that we won't run out of bytes in the bit_reader's slice?
| continue; | ||
| // The next loop iteration will call bit_reader.refill(), so verify that there | ||
| // are enough bytes available for that. | ||
| if bit_reader.bytes_remaining() >= 8 { |
There was a problem hiding this comment.
This causes a small performance regression, but it doesn't show up as statistically significant on my system.
|
This is a work in progress, to try to address issue #305. I was able to ensure that 2 of the 3 calls to the function meet the safety preconditions. For the remaining call, the one where I currently have a TODO comment, hopefully someone more familiar with the code that calls |
No description provided.