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

From: Torsten Bögershausen <tboegi@web.de>
Date: Jun 17, 2014, 11:08 UTC
Message-ID: <53A02195.8080202@web.de>
In-Reply-To: <cover.1402990051.git.jmmahler@gmail.com>

On 2014-06-17 09.34, Jeremiah Mahler wrote:

Show 17 quoted lines

> 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".

And another thing:
 What does cache_name_compare(name, namelen, ce->name, len))
 in name-hash.c do?
 Isn't that the same function ?

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.

Thanks for the effort, cleaning up is needed.