Re: [PATCH v2 0/3] add strnncmp() function

[Jeremiah Mahler <jmmahler@gmail.com>]

Torsten,

On Tue, Jun 17, 2014 at 01:08:05PM +0200, Torsten Bögershausen wrote:
> On 2014-06-17 09.34, Jeremiah Mahler wrote:
> > Add a strnncmp() function which behaves like strncmp() except it takes
> > the length of both strings instead of just one.
> > 
> > Then simplify tree-walk.c and unpack-trees.c using this new function.
> > Replace all occurrences of name_compare() with strnncmp().  Remove
> > name_compare(), which they both had identical copies of.
> > 
> > Version 2 includes suggestions from Jonathan Neider [1]:
> > 
> >   - Fix the logic which caused the new strnncmp() to behave differently
> > 	from the old version.  Now it is identical to strncmp().
> > 
> >   - Improve description of strnncmp().
> > 
> > Also, strnncmp() was switched from using memcmp() to strncmp()
> > internally to make it clear that this is meant for strings, not
> > general buffers.
> I don't think this is a good change, for 2 reasons:
> - It changes the semantics of existing code, which should be carefully
>   reviewed, documented and may be put into a seperate commit.
> - Looking into the code for memcmp() and strncmp() in libc,
>   I can see that memcmp() is written in 13 lines of assembler,
>   (on a 386 system) with a fast
>     repz cmpsb %es:(%edi),%ds:(%esi)
>   working as the core engine.
>   
>   strncmp() uses 83 lines of assembler, because after each comparison
>   the code needs to check of the '\0' in both strings.
> - I can't see a reason to replace efficient code with less efficient code,
>   so moving the old function "as is" into a include file, and declare
>   it "static inline" could be the first step.
> 
>   Having code inline may open the door for the compiler to decide,
>   "Oh, I know exactly what memcmp() does, so I through in a handfull
>   of lines assembly code, instead of calling memcmp() from libc".
> 
Thanks for explaining the benefits of memcmp() over strcmp(), I will
switch it back.

The only case I can imagine where it would make a difference is when
there is a '\0' in the middle of the string.  But that would be an
unlikely case since it probably meant the lengths were mis-calculated.

> 
> And another thing:
>  What does cache_name_compare(name, namelen, ce->name, len))
>  in name-hash.c do?
>  Isn't that the same function ?
> 
cache_name_compare() is the same except it returns -1, +1 instead of -N,
+N.  However, none of the cases where name_compare() is used need the
magnitude so this function could be used.

> I like strnncmp() better than 
> cache_name_compare() or name_compare(),
> but I agree with Erik here that strnncmp() has the potential to
> become a name clash some day, so that git_strnncmp() may be better.
> 
Agreed.

> Thanks for the effort, cleaning up is needed.
> 

Thanks for the feedback :-)

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler