Conversation
Reviewer's GuideThis PR refactors the monolithic Class diagram for LengthPrefixedProcessor and related typesclassDiagram
class LengthPrefixedProcessor {
-format: LengthFormat
+new(format: LengthFormat) -> Self
+decode(&self, src: &mut BytesMut) -> Result<Option<Vec<u8>>, std::io::Error>
+encode(&self, frame: &Vec<u8>, dst: &mut BytesMut) -> Result<(), std::io::Error>
}
class LengthFormat {
+bytes: usize
+endianness: Endianness
+new(bytes: usize, endianness: Endianness) -> Self
+read_len(&self, bytes: &[u8]) -> io::Result<usize>
+write_len(&self, len: usize, dst: &mut BytesMut) -> io::Result<()>
}
class Endianness {
<<enumeration>>
Big
Little
}
LengthPrefixedProcessor --> LengthFormat: has
LengthFormat --> Endianness: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/frame/conversion.rs:111` </location>
<code_context>
- }
- }
-
- out[size..].fill(0);
-
- Ok(size)
</code_context>
<issue_to_address>
Zeroing the remainder of the output buffer may be unnecessary.
Zeroing the unused portion may be redundant and could slightly affect performance. If preventing data leakage is the goal, document this or consider reducing the buffer size.
</issue_to_address>
### Comment 2
<location> `src/frame/tests.rs:67` </location>
<code_context>
- }
-
- #[rstest]
- fn u64_to_bytes_large() {
- let mut buf = [0u8; 8];
- let err = u64_to_bytes(300, 1, Endianness::Big, &mut buf)
- .expect_err("u64_to_bytes must fail if value exceeds 1-byte prefix");
- assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
- }
-
</code_context>
<issue_to_address>
Missing test for negative or zero-length values.
Consider adding a test for zero-length values to verify correct handling, especially if zero-length frames are valid in your protocol.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
use std::io;
=======
#[test]
fn u64_to_bytes_zero_length() {
let mut buf = [0u8; 8];
let err = u64_to_bytes(0, 0, Endianness::Big, &mut buf)
.expect_err("u64_to_bytes must fail if length is zero");
assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
}
use std::io;
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/frame/conversion.rs:49` </location>
<code_context>
-/// let buf = [0x00, 0x10, 0x20, 0x30];
-/// assert_eq!(bytes_to_u64(&buf, 2, Endianness::Big).unwrap(), 0x0010);
-/// ```
-pub fn bytes_to_u64(bytes: &[u8], size: usize, endianness: Endianness) -> io::Result<u64> {
- if !matches!(size, 1 | 2 | 4 | 8) {
- return Err(prefix_err(PrefixErr::UnsupportedSize));
</code_context>
<issue_to_address>
Consider simplifying the encoding and decoding logic by using direct slice copying and casting, and removing unnecessary helpers and error indirection.
You can collapse most of the per‐size/endianness arms into two simple copy+cast steps and drop both the `PrefixErr`/`prefix_err` indirection and the `parse_bytes` helper entirely. For example:
```rust
pub fn bytes_to_u64(
bytes: &[u8],
size: usize,
endianness: Endianness,
) -> io::Result<u64> {
if !matches!(size, 1 | 2 | 4 | 8) {
return Err(io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX));
}
if bytes.len() < size {
return Err(io::Error::new(io::ErrorKind::UnexpectedEof, ERR_INCOMPLETE_PREFIX));
}
let mut buf = [0u8; 8];
match endianness {
Endianness::Big => buf[8 - size..].copy_from_slice(&bytes[..size]),
Endianness::Little => buf[..size].copy_from_slice(&bytes[..size]),
}
let val = match endianness {
Endianness::Big => u64::from_be_bytes(buf),
Endianness::Little => u64::from_le_bytes(buf),
};
Ok(val)
}
```
```rust
pub fn u64_to_bytes(
len: usize,
size: usize,
endianness: Endianness,
out: &mut [u8; 8],
) -> io::Result<usize> {
if !matches!(size, 1 | 2 | 4 | 8) {
return Err(io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX));
}
// cast `len` into exactly the right integer type once
let prefix_bytes = match size {
1 => checked_prefix_cast::<u8>(len)?.to_be_bytes().to_vec(),
2 => checked_prefix_cast::<u16>(len)?.to_be_bytes().to_vec(),
4 => checked_prefix_cast::<u32>(len)?.to_be_bytes().to_vec(),
8 => checked_prefix_cast::<u64>(len)?.to_be_bytes().to_vec(),
_ => unreachable!(),
};
out.fill(0);
match endianness {
Endianness::Big => out[8 - size..].copy_from_slice(&prefix_bytes),
Endianness::Little => out[..size].copy_from_slice(&prefix_bytes),
};
Ok(size)
}
```
Steps to apply:
1. Remove the `PrefixErr` enum + `prefix_err` fn and inline `io::Error::new` calls.
2. Delete `parse_bytes` and its uses.
3. Replace both `bytes_to_u64` and `u64_to_bytes` with the above slice‐and‐cast logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/frame/conversion.rs (4)
10-25: Remove unnecessary error indirection.The
PrefixErrenum andprefix_errfunction add unnecessary complexity. Useio::Error::newdirectly with the error constants for cleaner code.
34-39: Remove theparse_byteshelper function.This generic helper adds complexity without significant benefit. The conversion logic can be simplified by using direct slice operations and casting.
49-67: Simplify the encoding logic using direct slice copying.Replace the complex match arms with simpler slice-and-cast logic as suggested in the past review. This will improve both performance and maintainability.
111-111: Document the purpose of zeroing unused buffer space.If this zeroing is intentional for security (preventing data leakage), document this clearly. Otherwise, remove it to avoid unnecessary performance overhead.
src/frame/tests.rs (1)
66-71: Add test coverage for zero-length values.Include a test case for zero-length values to verify correct error handling, as this is an important protocol edge case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/roadmap.md(1 hunks)src/frame.rs(0 hunks)src/frame/conversion.rs(1 hunks)src/frame/format.rs(1 hunks)src/frame/mod.rs(1 hunks)src/frame/processor.rs(1 hunks)src/frame/tests.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/frame.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.md
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/frame/mod.rssrc/frame/format.rssrc/frame/processor.rssrc/frame/conversion.rssrc/frame/tests.rs
🧬 Code Graph Analysis (3)
src/frame/mod.rs (2)
wireframe_testing/src/helpers.rs (1)
processor(22-22)src/frame/conversion.rs (2)
bytes_to_u64(49-68)u64_to_bytes(78-114)
src/frame/processor.rs (1)
src/frame/format.rs (2)
new(28-28)default(65-65)
src/frame/tests.rs (1)
src/frame/conversion.rs (2)
bytes_to_u64(49-68)u64_to_bytes(78-114)
🔇 Additional comments (12)
docs/roadmap.md (1)
27-27: LGTM - Documentation correctly updated to reflect modular structure.The path reference has been properly updated to match the new modular frame structure.
src/frame/mod.rs (1)
1-12: Excellent modular organisation and clean API design.The module structure is well-designed with proper documentation and selective re-exports that provide a cohesive interface whilst maintaining internal modularity.
src/frame/format.rs (1)
1-66: Well-designed format abstraction with excellent API ergonomics.The module provides a clean abstraction for length prefix formats with convenient constructors, proper error handling, and sensible defaults. The const constructors and pub(crate) visibility are appropriately used.
src/frame/tests.rs (1)
1-84: Excellent test coverage with comprehensive parameterisation.The test suite properly covers success cases, error conditions, and boundary cases using rstest effectively. The test organisation and error checking are exemplary.
src/frame/processor.rs (8)
1-1: Module documentation follows guidelines correctly.The module documentation uses the required
//!format and explains the module's purpose clearly.
8-26: FrameProcessor trait design is well-structured.The trait properly uses associated types for
FrameandError, includes comprehensive documentation with error conditions, and follows the Send + Sync bounds for concurrent usage. Error type constraints ensure proper error propagation.
28-41: FrameMetadata trait provides clean separation of concerns.The trait correctly separates metadata parsing from full frame decoding, which aligns with single responsibility principle. The return type
(Self::Frame, usize)properly indicates both the parsed frame and bytes consumed.
44-47: LengthPrefixedProcessor struct is appropriately minimal.The struct correctly encapsulates only the necessary
LengthFormatfield and derives appropriate traits (Clone, Copy, Debug) for value semantics.
49-53: Constructor follows coding guidelines.The
newfunction correctly uses#[must_use]attribute, is declared asconst fn, and follows the single-line function guideline by omittingreturn.
55-57: Default implementation delegates appropriately.The Default trait implementation correctly delegates to
LengthFormat::default()through the constructor, maintaining consistency.
63-76: Decode method handles edge cases properly.The implementation correctly:
- Checks buffer length before reading length prefix
- Uses
checked_addto prevent integer overflow- Returns
Ok(None)for incomplete frames (following tokio codec patterns)- Advances buffer position after reading length prefix
- Converts
BytesMuttoVec<u8>appropriatelyThe error handling uses the imported constant
ERR_FRAME_TOO_LARGEfor consistency.
78-84: Encode method implementation is correct.The method properly:
- Reserves buffer capacity upfront for efficiency
- Delegates length prefix writing to the format
- Extends buffer with frame bytes
- Uses appropriate error propagation with
?operatorThe implementation follows good practices for buffer management.
1 participant
Summary
framemodule into submodulestests.rsTesting
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_688b964c24a883228ecf4609c604eb2a
Summary by Sourcery
Refactor the frame module by splitting it into submodules for conversion, formatting, and processing, extract core frame encoding/decoding logic into dedicated helpers and traits, relocate unit tests to a separate file, and update documentation references accordingly.
Enhancements:
Documentation:
Tests: