From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 22 Aug 2018 14:28:56 GMT
Subject: Re: [ANNOUNCE] Git v2.19.0-rc0
Message-ID: <459f138f-78ba-6465-0f2b-b2419bbc36ff@gmail.com>
In-Reply-To: <20180822053626.GB535143@genre.crustytoothpaste.net>

On 8/22/2018 1:36 AM, brian m. carlson wrote:
> On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:
>> So I wonder if there's some other way to tell the compiler that we'll
>> only have a few values. An enum comes to mind, though I don't think the
>> enum rules are strict enough to make this guarantee (after all, it's OK
>> to bitwise-OR enums, so they clearly don't specify all possible values).
> I was thinking about this:
>
> diff --git a/cache.h b/cache.h
> index 1398b2a4e4..1f5c6e9319 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1033,7 +1033,14 @@ extern const struct object_id null_oid;
>   
>   static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
>   {
> -	return memcmp(sha1, sha2, the_hash_algo->rawsz);
> +	switch (the_hash_algo->rawsz) {
> +		case 20:
> +			return memcmp(sha1, sha2, 20);
> +		case 32:
> +			return memcmp(sha1, sha2, 32);
> +		default:
> +			assert(0);
> +	}
>   }
>   
>   static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
>
> That would make it obvious that there are at most two options.
> Unfortunately, gcc for me determines that the buffer in walker.c is 20
> bytes in size and steadfastly refuses to compile because it doesn't know
> that the value will never be 32 in our codebase currently.  I'd need to
> send in more patches before it would compile.
>
> I don't know if something like this is an improvement or now, but this
> seems to at least compile:
>
> diff --git a/cache.h b/cache.h
> index 1398b2a4e4..3207f74771 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;
>   
>   static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
>   {
> -	return memcmp(sha1, sha2, the_hash_algo->rawsz);
> +	switch (the_hash_algo->rawsz) {
> +		case 20:
> +		case 32:
> +			return memcmp(sha1, sha2, the_hash_algo->rawsz);
> +		default:
> +			assert(0);
> +	}
>   }
In my testing, I've had the best luck with this change:

diff --git a/cache.h b/cache.h
index b1fd3d58ab..6c8b51c390 100644
--- a/cache.h
+++ b/cache.h
@@ -1023,7 +1023,14 @@ extern const struct object_id null_oid;

  static inline int hashcmp(const unsigned char *sha1, const unsigned 
char *sha2)
  {
-       return memcmp(sha1, sha2, the_hash_algo->rawsz);
+       switch (the_hash_algo->rawsz) {
+               case 20:
+                       return memcmp(sha1, sha2, 20);
+               case 32:
+                       return memcmp(sha1, sha2, 32);
+               default:
+                       assert(0);
+       }
  }

The fact that '20' and '32' are constants here may be helpful to the 
compiler. Can someone else test the perf?

Thanks,
-Stolee