Re: [PATCH v1 1/4] hashmap: factor out getting an int hash code from a, SHA1

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2014, 17:22 UTC
Message-ID: <xmqqoax1dqxb.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53B485A4.20609@gmail.com>

Karsten Blees <karsten.blees@gmail.com> writes:

> Copying the first bytes of a SHA1 is duplicated in six places, however,
> the implications (wrong byte order on little-endian systems) is documented
> only once.

s/wrong /different /; but other than that I think this is a good change.

Show 8 quoted lines

> +`unsigned int sha1hash(const unsigned char *sha1)`::
> +
> +	Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash code
> +	for use in hash tables. Cryptographic hashes are supposed to have
> +	uniform distribution, so in contrast to `memhash()`, this just copies
> +	the first `sizeof(int)` bytes without shuffling any bits. Note that
> +	the results will be different on big-endian and little-endian
> +	platforms, so they should not be stored or transferred over the net!

Tone down with s/!/./, perhaps?

Another thing we may want to caution against is to use it as a tie-breaker that affects the final outcome the user can observe, but that may be something that goes without saying. I dunno..

Show 14 quoted lines

> diff --git a/hashmap.h b/hashmap.h
> index a816ad4..ed5425a 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -13,6 +13,17 @@ extern unsigned int strihash(const char *buf);
>  extern unsigned int memhash(const void *buf, size_t len);
>  extern unsigned int memihash(const void *buf, size_t len);
>  
> +static inline unsigned int sha1hash(const unsigned char *sha1)
> +{
> +	/*
> +	 * Equivalent to 'return *(int *)sha1;', but safe on platforms that
> +	 * don't support unaligned reads.
> +	 */

s/int/unsigned &/; other than that, the explanation is good.

Show 5 quoted lines

> +	unsigned int hash;
> +	memcpy(&hash, sha1, sizeof(hash));
> +	return hash;
> +}
> +