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@xxxxxxxxx http://github.com/jmahler -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html