QLv2 alex's proposed changes - #74
Merged
Merged
Conversation
Splitting through `&mut self` means the reader never has to move `B` out of itself, so `ManuallyDrop` and the `Drop` impl go with it. Losing the `Drop` impl ends the reader's borrow of `bytes` in `fsm::receive` at its last use, making the `drop(reader)` there dead code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plain integer codecs are fixed width, so `decode::<u64>()` and `decode_varint::<u64>()` are one word apart and produce different wire formats, with nothing in a bare `u64` field to say which one applies. The `VarInt` trait is renamed `Primitive` so it doesn't read as a typo for the `Varint` struct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Groundwork for generating these with a macro: `encoded_len` sums the fields instead of restating a constant, discriminant enums all fail with ql_codec::Error, and both byte-container encode bounds are now BufView. Nothing checked `encoded_len` against `encode` before, since encode_record_vec only uses it to size the buffer. The new test covers every type and passes against the pre-change impls, so this is byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Most Encode/Decode impls just walked the fields in order. What stays hand-written reads or writes something that isn't a field: a flags byte (StreamData), a trailing optional (Ik1), a block count (RecordAck), or the rest of the buffer (EncryptedMessage, EncryptedPeerBundle). PairingInvite loses its codec entirely, since nothing ever called it, and array_wrapper is gone because codec_newtype covers it without #[macro_use]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MIN_WIRE_SIZE was a sum of maximums subtracted from the remaining record capacity, so the name had it backwards, and bounding the length prefixes as usize made it 41 on the host but 31 on the 32-bit target, giving the simulator and the device different framing. Three other WIRE_SIZE constants lost their last caller when `encoded_len` started summing fields; the survivors are the ones no value is in hand for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A usize varint is capped at ten bytes on a 64-bit host and five on the 32-bit target, so a length a host emits can be rejected outright by the device's decoder. Only per-record lengths and counts move; offsets, sequence numbers and ack spans stay u64, since those accumulate. Encoded bytes are unchanged for anything under 4 GiB, as a varint does not depend on the width it was declared with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The payload budget reserved room for the header's length prefix but not the header itself, so a stream opened with a header larger than the remaining record space built a frame that would not fit, and pushing it tripped the "builder has capacity" assert. Reserving stops once offset 0 is acked rather than once the header is sent, because until then a loss there is retransmitted and carries the header again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| /// Length prefixes are `u32`, so a 64-bit host cannot emit one a 32-bit peer would reject: a | ||
| /// `usize` varint is capped at ten bytes on one target and five on the other. | ||
| fn len_prefix(len: usize) -> u32 { |
Contributor
There was a problem hiding this comment.
yeah this is a good idea.
| @@ -0,0 +1,381 @@ | |||
| /// Defines a newtype and encodes it exactly as the value it wraps. | |||
| #[macro_export] | |||
| macro_rules! codec_newtype { | |||
Contributor
There was a problem hiding this comment.
I think we should just have a single codec! macro and then it matches the relevant branch automatically
| pub unsafe trait ByteSlice: Deref<Target = [u8]> + Sized { | ||
| /// splits the current byte view at `mid` without checking bounds | ||
| /// mid can be 0 | ||
| pub trait ByteSlice: Deref<Target = [u8]> + Sized { |
|
|
||
| impl RecordHeader { | ||
| pub const WIRE_SIZE: usize = size_of::<u8>() + RouteHeader::WIRE_SIZE + size_of::<u8>(); | ||
| pub const WIRE_SIZE: usize = size_of::<u8>() + QID::SIZE * 2 + size_of::<u8>(); |
Contributor
There was a problem hiding this comment.
hm is this intentional?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.