Re: [PATCH 6/6] Document some functions defined in object.c
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Feb 24, 2014, 08:47 UTC
- Message-ID
- <530B073D.5010702@alum.mit.edu>
- In-Reply-To
- <alpine.LFD.2.11.1402211222270.17677@knanqh.ubzr>
Nicolas, thanks for the very fast feedback!
On 02/21/2014 06:33 PM, Nicolas Pitre wrote:
Show 37 quoted lines
> 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/