Copilot commented on code in PR #336:
URL: https://github.com/apache/avro-rs/pull/336#discussion_r2524819722
##########
avro/src/util.rs:
##########
@@ -100,25 +105,34 @@ pub(crate) fn zag_i64<R: Read>(reader: &mut R) ->
AvroResult<i64> {
})
}
-fn encode_variable<W: Write>(mut z: u64, mut writer: W) -> AvroResult<usize> {
+/// Write the number as a varint to the writer.
+///
+/// Note: this function does not do zigzag encoding, for that see [`zig_i32`]
and [`zig_i64`].
+fn encode_variable<W: Write>(mut zigzagged: u64, mut writer: W) ->
AvroResult<usize> {
+ // Ensure the number is little endian for the varint encoding (no-op on LE
systems)
+ zigzagged = zigzagged.to_le();
Review Comment:
The addition of `to_le()` here is incorrect and will break varint encoding
on big-endian systems.
Varint encoding is endian-independent - it encodes numbers by extracting
7-bit chunks from least to most significant, which works correctly regardless
of the system's endianness. The original code was correct.
On a big-endian system, calling `to_le()` would byte-swap the value before
encoding, producing incorrect output. On little-endian systems, it's a no-op
but adds unnecessary operations.
**Recommendation:** Remove the `to_le()` call and the associated comment on
line 112.
```suggestion
```
##########
avro/src/util.rs:
##########
@@ -140,7 +154,7 @@ fn decode_variable<R: Read>(reader: &mut R) ->
AvroResult<u64> {
}
}
- Ok(i)
+ Ok(u64::from_le(i))
Review Comment:
The addition of `from_le()` here is incorrect and will break varint decoding
on big-endian systems.
The varint decoding algorithm already correctly reconstructs the value by
shifting and ORing 7-bit chunks, which is endian-independent. Calling
`from_le()` assumes the result is in little-endian format, which it isn't -
it's just the correctly reconstructed value.
On a big-endian system, this would byte-swap the correctly decoded value,
producing incorrect results. On little-endian systems, it's a no-op but adds
unnecessary operations.
**Recommendation:** Remove the `from_le()` call and keep it as `Ok(i)`.
```suggestion
Ok(i)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
