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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 6, 2016 at 2:27 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Jan 6, 2016 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>       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);
>>
>> 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?
>
> This is done based on Eric's suggestion [1]. Although its a little off normal
> convention. I find it small and simple. So I'm okay with either, your suggested
> change or the existing code.

A "copylen" variable would probably result in the clearest code since
it states explicitly what an otherwise opaque expression like (!!end *
!!omit_term) means, thus is easier to reason about.