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

refactor binary interface types from usize to their specific sizes #70

Closed
FreddieRidell opened this issue May 13, 2019 · 2 comments
Closed

Comments

@FreddieRidell
Copy link
Contributor

Feature Request

Summary

In some places usize is being used in situations where it would be better to explicitly define what bit size we want to use. One example can be found here:

// TODO: This will blow up on 32 bit systems, because usize can be 32 bits.
let length = reader.read_u64::<BigEndian>()? as usize;

Motivation

The crate is currently built with a 64 bit word size in mind, this will cause issues when run on a 32-bit system

Explanation

Review all uses of usize, evaluate if u64 would be more appropriate, change the types, fix any compiler errors that occour. (optional: build and run on a 32 bit system to check compatability)

Unresolved Questions

Is this something that should be done now? are there valid reasons to postpone this work? Are there any other relevant issues that have not been described in this ticket

@yoshuawuyts
Copy link
Contributor

@FreddieRidell I fully agree with you here, and would welcome patches to change this. We've done this in a few other crates too already. There shouldn't be any blockers to landing such changes.

@FreddieRidell
Copy link
Contributor Author

I've put up a PR, I had a look through the codebase and this was the only place where we obviously should use u64 over usize.

Unfortunately it seems there's no way to easily test 32-bit on travis-ci, and I don't have any way to test it available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants