Fix varint decoding - #3
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the serialization and deserialization logic for VarInt and VarLong by introducing named constants and updating the loop structures. However, the loop termination conditions in the write_to methods for both types are incorrect; they only check the 8th bit instead of verifying if any bits outside the 7-bit segment are set, which will cause encoding errors for values like 256.
| let mut val = self.0 as u32; | ||
| loop { | ||
| if (val & !PART) == 0 { | ||
| if (val & VARINT_CONTINUE_BIT) == 0 { |
There was a problem hiding this comment.
The loop termination condition (val & VARINT_CONTINUE_BIT) == 0 is incorrect. VARINT_CONTINUE_BIT is 0x80. This condition only checks if the 8th bit of val is zero. If val is 256 (0x100), the 8th bit is zero, but the value still requires multiple bytes to encode. This will result in incorrect encoding for any value where the 8th bit is 0 but higher bits are set. The condition should check if any bits outside the 7-bit segment are set to determine if more bytes are needed.
| if (val & VARINT_CONTINUE_BIT) == 0 { | |
| if (val & !VARINT_SEGMENT_BITS) == 0 { |
| let mut val = self.0 as u64; | ||
| loop { | ||
| if (val & !PART) == 0 { | ||
| if (val & VARLONG_CONTINUE_BIT) == 0 { |
There was a problem hiding this comment.
Similar to the VarInt implementation, the condition (val & VARLONG_CONTINUE_BIT) == 0 is incorrect for VarLong. It will cause values like 256 to be encoded incorrectly as a single byte because it only checks the 8th bit instead of all bits above the first 7. It should check if the value fits within the 7-bit segment.
| if (val & VARLONG_CONTINUE_BIT) == 0 { | |
| if (val & !VARLONG_SEGMENT_BITS) == 0 { |
Might need to check if it won't break anything else.