-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support isize and usize #72
Conversation
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.
Looks great; thanks!
@@ -212,6 +213,8 @@ impl Formatter { | |||
/// leb64-encode `x` and write it to self.bytes | |||
#[doc(hidden)] | |||
pub fn leb64(&mut self, x: u64) { | |||
// FIXME: Avoid 64-bit arithmetic on 32-bit systems. This should only be used for | |||
// pointer-sized values. |
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.
could you create a low priority issue for this work?
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.
Hmm, timestamps are u64
and use leb64, so this isn't quite correct. Should we make timestamps usize
s?
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.
leb64
is LEB128 with 64-bit arithmetic. The 10 byte buffer should be large enough for 64 bit values, no? Why make the timestamps usize
which are 32-bit values on the target side?
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.
Yeah that works, but if leb64 would only be used on usize-sized values, we could avoid 64-bit arithmetic as well and just do it with 32-bit (assuming a 32-bit target). This is currently almost the case, except that the 64-bit timestamps also use leb64.
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 see what you mean. For alpha release I think we want 64-bit timestamps working.
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.
opened #75
No description provided.