Skip to content
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

fix: Remove as conversions in protocol/primitives.rs #118

Merged
merged 3 commits into from
Mar 11, 2022

Conversation

pierwill
Copy link
Contributor

Part of #101.

Comment on lines +559 to +560
let len = u64::try_from(self.0.len()).map_err(WriteError::Overflow)?;
UnsignedVarint(len).write(writer)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small note because we had a chat about integer sizes yesterday:

.len() is usually usize which on 64bit systems is the same as u64. So this is a no-op on 64bit systems (basically all desktops, laptops, servers and about all smartphones nowadays). For 32bit systems (embedded) this also wouldn't be an issue because we would convert to a wider integer. It would only be an issue if usize is 128bit (the only system I know that would allow that is the RISC-V 128bit architecture and there are no chips or compilers available for it) AND when we have more than 2^64 - 1 elements in that list (which would also be insane). So I think this is somewhat of an impossible error case, but on the other hand it also doesn't hurt since the method in question already returns a Result.

@crepererum crepererum added the automerge Instruct kodiak to merge the PR label Mar 11, 2022
@kodiakhq kodiakhq bot merged commit 2c13f4b into influxdata:main Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instruct kodiak to merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants