Re: [PATCH 1/2] Add strchrnul()
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Nov 9, 2007, 13:42 UTC
- Message-ID
- <fh1o4o$jei$1@ger.gmane.org>
- In-Reply-To
- <473434ED.50002@op5.se>
[Cc: Andreas Ericsson <ae@op5.se>,
René Scharfe <rene.scharfe@lsrfire.ath.cx>,
Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org]Andreas Ericsson wrote:
Show 25 quoted lines
> René Scharfe wrote: >> As suggested by Pierre Habouzit, add strchrnul(). It's a useful GNU >> extension and can simplify string parser code. There are several >> places in git that can be converted to strchrnul(); as a trivial >> example, this patch introduces its usage to builtin-fetch--tool.c. >> >> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> >> --- >> >> Makefile | 13 +++++++++++++ >> builtin-fetch--tool.c | 8 ++------ >> compat/strchrnul.c | 8 ++++++++ >> git-compat-util.h | 5 +++++ >> 4 files changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 0d5590f..578c999 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -30,6 +30,8 @@ all:: >> # >> # Define NO_MEMMEM if you don't have memmem. >> # >> +# Define NO_STRCHRNUL if you don't have strchrnul. >> +#
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...
Show 11 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?
Show 26 quoted lines
> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index 6a78517..ed60847 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -435,9 +435,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
> cp++;
> if (!*cp)
> break;
> - np = strchr(cp, '\n');
> - if (!np)
> - np = cp + strlen(cp);
> + np = strchrnul(cp, '\n');
> if (pass) {
> lrr_list[i].line = cp;
> lrr_list[i].name = cp + 41;
> @@ -461,9 +459,7 @@ static int pick_rref(int sha1_only, const char *rref, const char *ls_remote_resu
> rref++;
> if (!*rref)
> break;
> - next = strchr(rref, '\n');
> - if (!next)
> - next = rref + strlen(rref);
> + next = strchrnul(rref, '\n');
> rreflen = next - rref;
>
> for (i = 0; i < lrr_count; i++) {This IMHO should go to separate patch.
-- Jakub Narebski Warsaw, Poland ShadeHawk on #git