Re: [PATCH 2/9] strbuf: add strbuf_tolower function

From: Kyle J. McKay <mackyle@gmail.com>
Date: May 22, 2014, 00:07 UTC
Message-ID: <AD42BA87-3C73-4EB2-AF88-96C4F7E6FA0C@gmail.com>
In-Reply-To: <20140521102742.GB30464@sigill.intra.peff.net>

On May 21, 2014, at 03:27, Jeff King wrote:

Show 76 quoted lines

> This makes config's lowercase() function public.
>
> Note that we could continue to offer a pure-string
> lowercase, but there would be no callers (in most
> pure-string cases, we actually duplicate and lowercase the
> duplicate).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This ones gets used later on...
>
> Documentation/technical/api-strbuf.txt | 4 ++++
> config.c                               | 8 +-------
> strbuf.c                               | 7 +++++++
> strbuf.h                               | 1 +
> 4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/ 
> technical/api-strbuf.txt
> index 3350d97..8480f89 100644
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -125,6 +125,10 @@ Functions
>
> 	Strip whitespace from the end of a string.
>
> +`strbuf_tolower`::
> +
> +	Lowercase each character in the buffer using `tolower`.
> +
> `strbuf_cmp`::
>
> 	Compare two buffers. Returns an integer less than, equal to, or  
> greater
> diff --git a/config.c b/config.c
> index a30cb5c..03ce5c6 100644
> --- a/config.c
> +++ b/config.c
> @@ -147,12 +147,6 @@ int git_config_include(const char *var, const  
> char *value, void *data)
> 	return ret;
> }
>
> -static void lowercase(char *p)
> -{
> -	for (; *p; p++)
> -		*p = tolower(*p);
> -}
> -
> void git_config_push_parameter(const char *text)
> {
> 	struct strbuf env = STRBUF_INIT;
> @@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,
> 		strbuf_list_free(pair);
> 		return error("bogus config parameter: %s", text);
> 	}
> -	lowercase(pair[0]->buf);
> +	strbuf_tolower(pair[0]);
> 	if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
> 		strbuf_list_free(pair);
> 		return -1;
> diff --git a/strbuf.c b/strbuf.c
> index ee96dcf..a1b8a47 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)
> 	sb->buf[sb->len] = '\0';
> }
>
> +void strbuf_tolower(struct strbuf *sb)
> +{
> +	size_t i;
> +	for (i = 0; i < sb->len; i++)
> +		sb->buf[i] = tolower(sb->buf[i]);
> +}
> +

Wouldn't a direct transfer of the lowercase function be something more like:

void strbuf_tolower(struct strbuf *sb)
{
	char *p = sb->buf;
	for (; *p; p++)
		*p = tolower(*p);
}

That seems to me to be a bit more efficient.  According to the  
comments in strbuf.c, "people can always assume buf is non NULL and - 
 >buf is NUL terminated even for a freshly initialized strbuf."

Show 19 quoted lines

> struct strbuf **strbuf_split_buf(const char *str, size_t slen,
> 				 int terminator, int max)
> {
> diff --git a/strbuf.h b/strbuf.h
> index 39c14cf..6b6f745 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf  
> *sb, size_t len)
> extern void strbuf_trim(struct strbuf *);
> extern void strbuf_rtrim(struct strbuf *);
> extern void strbuf_ltrim(struct strbuf *);
> +extern void strbuf_tolower(struct strbuf *sb);
> extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
>
> /*
> -- 
> 2.0.0.rc1.436.g03cb729
>