Re: [PATCH 1/2] Add strchrnul()

[Andreas Ericsson <ae@op5.se>]

Jakub Narebski wrote:
> [Cc: Andreas Ericsson <ae@op5.se>, 
>      René Scharfe <rene.scharfe@lsrfire.ath.cx>, 
>      Junio C Hamano <gitster@pobox.com>,
>      git@vger.kernel.org]
> 

I'm not sure what's up with this, but I didn't see this email on git@vger,
so re-adding it to the Cc list now.

> 
> Original patch lacked adding appropriate test to configure,
> i.e. something like below to configure.ac
> 
>  #
>  # Define NO_STRCHRNUL if you don't have strchrnul.
>  AC_CHECK_FUNC(strchrnul,
>  [NO_STRCHRNUL=],
>  [NO_STRCHRNUL=YesPlease])
>  AC_SUBST(NO_STRCHRNUL)
>  
> and the following line to config.mak.in
> 
>  NO_STRCHRNUL=@NO_STRCHRNUL@
> 
>> This seems overly complicated. How about this instead?
> [...]
>> I'm fairly much against forcing people to know what library
>> functions they have in order to get software to compile
>> properly. This is, imo, a neater solution, and also inlines
>> the function as suggested by Dscho.
> 
> Wouldn't it be better to add ./configure check instead? See above.
> 
> Although I guess that people using ./configure to set compilation
> options (to generate config.mak.autogen) are minority...
> 

Perhaps. I know I don't anyway, and now it's become standard not to
do so for a significant part of the git-tracking world.

>> +#if !defined(__GLIBC__) && !__GLIBC_PREREQ(2, 1)
>> +# define strchrnul(s, c) gitstrchrnul(s, c)
>> +static inline char *gitstrchrnul(const char *s, int c)
>> +{
>> +       while (*s && *s != c)
>> +               s++;
>> +
>> +       return (char *)s;
>> +}
>> +#endif
>> +
> 
> This is good solution, although I'm not sure about the check itself.
> What if somebody has libc which is not glibc, but it does have
> strchrnul?
> 

They most likely fall into the minority that get to suffer from
using code that's as fast as what's in there today, although
a bit more readable. The glibc optimization is really only
worthwhile for architectures where strchrnul()-like operations
have microcode support, or when it's used on large strings.

YMMV. I suppose rewriting it as

#if defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 1)
# define HAVE_STRCHRNUL
#endif

#ifdef HAVE_STRCHRNUL
...

would work too, and will provide an easier way out for other fellas
wanting to say "Hey, my favourite solaris libc has this too!". OTOH,
that rewrite can be done when the first such case appears.


>> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> 
> This IMHO should go to separate patch.
> 

*shrug* Rene had it in his. Monkey see monkey do ;-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231