Re: [PATCH 6/6] Document some functions defined in object.c

[Michael Haggerty <mhagger@alum.mit.edu>]

Nicolas, thanks for the very fast feedback!

On 02/21/2014 06:33 PM, Nicolas Pitre wrote:
> On Fri, 21 Feb 2014, Michael Haggerty wrote:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> Minor nits below.
> 
> 
>> ---
>>  object.c | 23 ++++++++++++++++++++++-
>>  object.h |  7 +++++++
>>  2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/object.c b/object.c
>> index 584f7ac..c34e225 100644
>> --- a/object.c
>> +++ b/object.c
>> @@ -43,14 +43,26 @@ int type_from_string(const char *str)
>>  	die("invalid object type \"%s\"", str);
>>  }
>>  
>> +/*
>> + * Return a numerical hash value between 0 and n-1 for the object with
>> + * the specified sha1.  n must be a power of 2.
>> + *
>> + * Since the sha1 is essentially random, we just take the required
>> + * bits from the first sizeof(unsigned int) bytes of sha1.
> 
> This might be improved a little.  The only reason for the sizeof() is 
> actually to copy those bits into a properly aligned integer.  Some 
> architectures have alignment restrictions that incure a significant cost 
> when integer operations are performed on unaligned data whereas sha1 
> pointers don't have any particular alignment requirements.  Once upon a 
> time this used to simply be:
> 
> 	return *(unsigned int *)sha1 & (n - 1);
> 
> The memcpy is there only to avoid unaligned accesses.

I understand all that; it's clear that the old code is not correct C,
isn't it?  And ISTM that the use of memcpy() is an implementation detail
that is not relevant to callers and so not needed in the docstring.  So
I suggest that your note be added as comments within the function; what
do you think?

Contrariwise, I thought about it again and believe that it *is*
important for the docstring to mention explicitly that the return value
is architecture-dependent (it depends on endianness and possibly
sizeof(unsigned int)).  Presumably the function is only used within one
Git invocation, so this isn't a problem, but we should warn callers.
Alternatively, we could stick a call to ntohl() in the function to make
the return value consistent, but this would cost a little bit on
little-endian computers.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/