Re: [PATCH v2 1/8] pack-objects: create new name-hash function version

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> "Jonathan Tan via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> diff --git a/pack-objects.h b/pack-objects.h
>> index b9898a4e64b..15be8368d21 100644
>> --- a/pack-objects.h
>> +++ b/pack-objects.h
>> @@ -207,6 +207,34 @@ static inline uint32_t pack_name_hash(const char *name)
>>  	return hash;
>>  }
>>  
>> +static inline uint32_t pack_name_hash_v2(const char *name)
>> +{
>> +	uint32_t hash = 0, base = 0, c;
>> + ...
>> +	while ((c = *name++)) {
>> + ...
>> +			/*
>> +			 * 'c' is only a single byte. Reverse it and move
>> +			 * it to the top of the hash, moving the rest to
>> +			 * less-significant bits.
>> +			 */
>> +			c = (c & 0xF0) >> 4 | (c & 0x0F) << 4;
>> + ...
> This works because `c` is masked before any arithmetic is performed on
> it, but the cast from potentially signed char to uint32_t still makes
> me nervous - if char is signed, it behaves as if it was first cast to
> int32_t and only then to uint32_t, ...
> I would declare `c` as uint8_t or unsigned char.

I think you meant the type of "name", and your worry is that *name
may pick up a negative integer from there when the platform char is
signed?  Here we are dealing with a pathname that has often UTF-8
encoded non-ASCII letters with 8th bit set, and I agree with you
that being explicitly unsigned would certainly help reduce the
cognitive load.

Thanks.