Short writes

[laurmaedje]

While browsing the code, I was wondering whether the short writes are actually exposed. Since even though the generic Hasher<S> implements them, the three wrapping hashers don't forward to these impls. Am I missing something or is this intentional? I found an issue on the rust repo from a while ago discussing hurt compile times due to the short writes, but then again there was this commit for faster short writes.

[jedisct1]

By adding use std::hash::Hasher;, function such as write_u64() are available, since the sip::* and sip128::* structures implement Hasher.

Is anything else required?

[laurmaedje]

Yes, they are available, but I think they are available due to std's default implementation, instead of the custom short_write implementation. So the implementation is something like this:

fn write_u64(&mut self, i: u64) {
    self.hasher.write(&i.to_ne_bytes())
}

Instead of the (maybe) more efficient:

fn write_u64(&mut self, i: u64) {
    self.hasher.short_write(i, i.to_le() as u64);
}

So this is merely (if at all) a performance and dead code concern.

[jedisct1]

rust-siphash/src/sip.rs

Lines 310 to 329 in 71dd491

	impl<S: Sip> hash::Hasher for Hasher<S> {
	#[inline]
	fn write_usize(&mut self, i: usize) {
	self.short_write(i, i.to_le() as u64);
	}
	#[inline]
	fn write_u8(&mut self, i: u8) {
	self.short_write(i, i as u64);
	}
	#[inline]
	fn write_u32(&mut self, i: u32) {
	self.short_write(i, i.to_le() as u64);
	}
	#[inline]
	fn write_u64(&mut self, i: u64) {
	self.short_write(i, i.to_le() as u64);
	}

Isn't it what this code does?

[laurmaedje]

For the type Hasher<S>, yes. But that type isn't exposed and the Hash impls of the three public wrapper types SipHasher, SipHasher13 and SipHasher24 don't forward to these methods and therefore automatically get std's implementation.

[jedisct1]

I must confess that I don't understand the problem you are trying to describe, but if you have a PR to fix it, that will make everything clear :)

[laurmaedje]

I was just about to do a PR, but this is exactly what I meant :)

[jedisct1]

The code duplication is really awful :(

[laurmaedje]

Yes, but then again there's also a lot of duplication between sip.rs and sip128.rs. So maybe the duplication could be cut in half? And I guess SipHasher could also just be a type alias to SipHasher24 to remove even more duplication.