Re: [PATCH 1/2] Add strchrnul()
- From
Andreas Ericsson <ae@op5.se>
- Date
- Nov 9, 2007, 13:59 UTC
- Message-ID
- <473467C4.8020208@op5.se>
- In-Reply-To
- <fh1o4o$jei$1@ger.gmane.org>
Jakub Narebski wrote:
Show 5 quoted lines
> [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.
Show 27 quoted lines
> > 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.
Show 16 quoted lines
>> +#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