Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements missing serialize_bytes and deserialize_bytes functionality while fixing edge cases and adding comprehensive test coverage for the serde binary serialization library.
- Replaces
unimplemented!()placeholders with working implementations for byte serialization/deserialization - Adds extensive test coverage for edge cases like empty collections, various enum variants, and borrowed bytes
- Updates version to 1.1.0 and adds serde_bytes dependency
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/serde_binary_adv/stream/ser.rs | Implements serialize_bytes method by delegating to serialize_vec |
| src/serde_binary_adv/stream/de.rs | Implements deserialize_bytes and deserialize_byte_buf methods with proper memory management |
| src/serde_binary_adv/stream.rs | Adds comprehensive test coverage for bytes, empty collections, and enum variants |
| src/serde_binary_adv/ser.rs | Implements serialize_bytes with special handling for empty byte slices |
| src/serde_binary_adv/de.rs | Fixes deserialize_bytes to use visit_borrowed_bytes instead of visit_bytes |
| src/serde_binary_adv.rs | Adds tests for serde_bytes integration and borrowed byte references |
| Cargo.toml | Updates version to 1.1.0 and adds serde_bytes dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| unimplemented!() | ||
| let len = self.next_usize()?; | ||
| let vec = self.take(len)?; | ||
| let bytes: &'de [u8] = Box::leak(vec.into_boxed_slice()); |
There was a problem hiding this comment.
Using Box::leak creates a memory leak by intentionally leaking heap-allocated memory. This violates memory safety principles and will cause unbounded memory growth. Consider using a different approach that doesn't leak memory, such as storing the Vec in a field of the deserializer or using a different lifetime management strategy.
| fn serialize_bytes(self, _v: &[u8]) -> Result<Self::Ok> { | ||
| unimplemented!() | ||
| fn serialize_bytes(self, v: &[u8]) -> Result<Self::Ok> { | ||
| if v.len() > 0 { |
There was a problem hiding this comment.
Using v.len() > 0 is less idiomatic than !v.is_empty(). The is_empty() method is more explicit about the intent and is the preferred way to check for empty collections in Rust.
| if v.len() > 0 { | |
| if !v.is_empty() { |
| fn roundtrip<'de>(input: &'de [u8]) { | ||
| let deserialized: &'de [u8] = Deserializer::from_bytes(&input, false).unwrap(); | ||
| assert_eq!(&input[1..], deserialized); | ||
| } | ||
|
|
||
| roundtrip(&serialized); |
There was a problem hiding this comment.
The assertion compares &input[1..] (skipping the first byte) with deserialized, but the test should compare the original value with deserialized. This appears to be accounting for a length prefix, but the logic is unclear and may not be testing the correct behavior.
| fn roundtrip<'de>(input: &'de [u8]) { | |
| let deserialized: &'de [u8] = Deserializer::from_bytes(&input, false).unwrap(); | |
| assert_eq!(&input[1..], deserialized); | |
| } | |
| roundtrip(&serialized); | |
| fn roundtrip<'de>(value: &'de [u8], input: &'de [u8]) { | |
| let deserialized: &'de [u8] = Deserializer::from_bytes(&input, false).unwrap(); | |
| assert_eq!(value, deserialized); | |
| } | |
| roundtrip(value, &serialized); |
|
|
||
| let serialized = Serializer::to_bytes(&value, false).unwrap(); | ||
| let deserialized: &Bytes = | ||
| Deserializer::from_bytes(Bytes::new(&serialized), false).unwrap(); |
There was a problem hiding this comment.
The deserialization is attempting to deserialize from Bytes::new(&serialized) but should deserialize from &serialized directly. The Bytes::new(&serialized) creates a Bytes wrapper around the serialized data, which is not the correct input format for the deserializer.
| Deserializer::from_bytes(Bytes::new(&serialized), false).unwrap(); | |
| Deserializer::from_bytes(&serialized, false).unwrap(); |
Fixed minor edge cases; implemented missing *bytes functions; improved test coverage