From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 07 Apr 2017 18:04:57 GMT
Subject: Re: [PATCH v6 1/3] read-cache: add strcmp_offset function
Message-ID: <2244d0d6-5909-a5b1-6606-4b22331dca59@jeffhostetler.com>
In-Reply-To: <97be34a6-ecdb-9820-1315-4cb5ed3b9101@web.de>



On 4/6/2017 7:07 PM, René Scharfe wrote:
> Am 06.04.2017 um 18:34 schrieb git@jeffhostetler.com:
>> diff --git a/read-cache.c b/read-cache.c
>> index 9054369..e8f1900 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -887,6 +887,26 @@ static int has_file_name(struct index_state *istate,
>>       return retval;
>>   }
>>   +
>> +/*
>> + * Like strcmp(), but also return the offset of the first change.
>> + * If strings are equal, return the length.
>> + */
>> +int strcmp_offset(const char *s1, const char *s2, int *first_change)
>> +{
>> +    int k;
>> +
>> +    if (!first_change)
>> +        return strcmp(s1, s2);
>> +
>> +    for (k = 0; s1[k] == s2[k]; k++)
>> +        if (s1[k] == '\0')
>> +            break;
>> +
>> +    *first_change = k;
>> +    return ((unsigned char *)s1)[k] - ((unsigned char *)s2)[k];
>> +}
>
> I like how this is shaping up to become a reusable function, but I only
> found one other possible use case (in read-cache.c::ce_write_entry),
> which somehow irritates me.  Either I didn't look hard enough (likely),
> we haven't got fitting code just yet, or this function isn't meant to be
> exported -- in the latter case its interface doesn't have to be polished
> as much.  (Just a thought, don't let me stop you.)
>
> Assuming strcmp_offset() is a library-grade function then its
> first_change parameter should point to a size_t instead of an int, no?

yes, size_t. thanks.

>
> Casting away the const qualifier in the return line is a bit iffy.  Why
> not cast after dereferencing, like this?
>
>     return (unsigned char)s1[k] - (unsigned char)s2[k];

Good point.

Thanks
Jeff