-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
size fix #120
Conversation
hchataing
commented
Nov 26, 2024
- Missing () in generated payload size encoder
- Add semantic test for payload size field encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the changes and it's not actually clear to me which of them fixes anything?
@@ -457,7 +457,7 @@ impl Packet for GrandChild { | |||
maximum_size: 0xff, | |||
}); | |||
} | |||
buf.put_u8(2 + self.payload.len() as u8); | |||
buf.put_u8((2 + self.payload.len()) as u8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks like it could be a fix. However, on a second look,
(2 + self.payload.len()) as u8
and
2 + (self.payload.len() as u8)
should give the same result, right? Will the 2
literal not be type-inferred to be a u8
here?
Is the problem with very large integers (larger than i32
?) which can overflow in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is more apparent on this declaration:
little_endian_packets
struct Elem {
_size_(value) : 8,
value : 8[],
}
packet Parent {
_size_(_payload_) : 8,
_payload_,
}
packet Child : Parent {
b : Elem,
d : Elem,
}
The generated code for <Child as Packet>::encode
contained the following line which was missing parentheses
buf.put_u8(self.b.encoded_len() + self.d.encoded_len() as u8);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the added semantic test to properly check this output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now I see the problem clearly! Thanks 🙂