Re: [PATCH v3 01/15] strbuf: introduce strbuf_split_str_omit_term()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2016, 19:24 UTC
Message-ID: <xmqq7fjnkexv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1451980994-26865-2-git-send-email-Karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 41 quoted lines

> The current implementation of 'strbuf_split_buf()' includes the
> terminator at the end of each strbuf post splitting. Add an option
> wherein we can drop the terminator if desired. In this context
> introduce a wrapper function 'strbuf_split_str_omit_term()' which
> splits a given string into strbufs without including the terminator.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  strbuf.c |  7 ++++---
>  strbuf.h | 25 ++++++++++++++++---------
>  2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index b552a13..b62508e 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -115,7 +115,7 @@ void strbuf_tolower(struct strbuf *sb)
>  }
>  
>  struct strbuf **strbuf_split_buf(const char *str, size_t slen,
> -				 int terminator, int max)
> +				 int terminator, int max, int omit_term)
>  {
>  	struct strbuf **ret = NULL;
>  	size_t nr = 0, alloc = 0;
> @@ -123,14 +123,15 @@ struct strbuf **strbuf_split_buf(const char *str, size_t slen,
>  
>  	while (slen) {
>  		int len = slen;
> +		const char *end = NULL;
>  		if (max <= 0 || nr + 1 < max) {
> -			const char *end = memchr(str, terminator, slen);
> +			end = memchr(str, terminator, slen);
>  			if (end)
>  				len = end - str + 1;
>  		}
>  		t = xmalloc(sizeof(struct strbuf));
>  		strbuf_init(t, len);
> -		strbuf_add(t, str, len);
> +		strbuf_add(t, str, len - !!end * !!omit_term);

You initialize with "len" but sometimes copy less than that, which looks somewhat sloppy.

Maybe I am old-fashioned, but use of a multiplication when you do not mean to numerically multiply but merely to perform a logical operation made me go "Huh?".

Perhaps using another variable would make it easier to follow? Either using a boolean that tells us that the terminating byte is to be omitted, i.e.

	int len = slen;
        int omit = 0;
        if ( ... we are still splitting ... ) {
		const char *end = memchr(...);
                if (end) {
                	len = end - str + 1;
			omit = !!omit_term;
		}
	}
	strbuf_init(t, len - omit);
        strbuf_add(t, str, len - omit);

or an integer "copylen" that tells us how many bytes to copy, which often is the same as "len" but sometimes different by 1 byte?