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 Endianness / portability ? #3

Open
dhardy opened this issue Nov 22, 2017 · 3 comments
Open

Fix Endianness / portability ? #3

dhardy opened this issue Nov 22, 2017 · 3 comments

Comments

@dhardy
Copy link
Contributor

dhardy commented Nov 22, 2017

I notice that this uses the simplest implementation of Hasher possible. Functions like write_u16 cast multi-byte numbers to byte-slices without fixing Endianness; this implies one should get different results on Big Endian vs Little Endian machines.

I don't know whether this is the only portability issue (e.g. I don't know if there are any complications if the bytes supplied to write are not sufficiently aligned; presumably only performance).

Now, I'm not sure whether to call the lack of Endianness fixing in Hasher default function impls a bug in the standard library or not, but it's a problem for our use-case (not certain whether we will use MetroHash yet). We could simply wrap with HasherToLE (endian-hasher), but that seems messy.

Your thoughts?

@arthurprs
Copy link
Owner

arthurprs commented Nov 22, 2017

It may be a matter of adding some to_le() to the read functions in utils.rs. Not all hashers are portable so I'd say the Hash trait is fine.

Is there any easy way of testing such things in a x64 laptop?

@dhardy
Copy link
Contributor Author

dhardy commented Nov 23, 2017

Best option may be cross-compiling on Travis (see trust; you can remove parts of the .travis.yml script, e.g. deployment), and unit tests.

@dhardy
Copy link
Contributor Author

dhardy commented Nov 23, 2017

I think fixing Endianness is mostly about calling .to_le() in the Hasher functions as here.

There is a note on portability at the bottom of the original implementation's README. Whether you want to differ from his direction here I guess you have to decide — if you do, results won't be compatible (on BE machines, which may be irrelevant). The wrapper mentioned above is another possibility for users, but this should be documented.

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