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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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?