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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 25, 2016, 06:12 UTC
Message-ID: <CAOLa=ZRhViUntR4yO66imd7D6KN+CCqkREGPVRfTfyN3zLQoAw@mail.gmail.com>
In-Reply-To: <CAPig+cQ2-ddENz4dKsvTW-tOrwugUGa4g-3PySMTBsHsrtO93w@mail.gmail.com>

On Fri, Jan 22, 2016 at 1:17 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 43 quoted lines

> 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.

Sure, I think this would do:

diff --git a/strbuf.c b/strbuf.c
index b552a13..81e279d 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,18 @@ struct strbuf **strbuf_split_buf(const char
*str, size_t slen,

        while (slen) {
                int len = slen;
+               int copylen = len;
+               const char *end = NULL;
                if (max <= 0 || nr + 1 < max) {
-                       const char *end = memchr(str, terminator, slen);
-                       if (end)
+                       end = memchr(str, terminator, slen);
+                       if (end) {
                                len = end - str + 1;
+                               copylen = len - !!omit_term;
+                       }
                }
                t = xmalloc(sizeof(struct strbuf));
-               strbuf_init(t, len);
-               strbuf_add(t, str, len);
+               strbuf_init(t, copylen);
+               strbuf_add(t, str, copylen);
                ALLOC_GROW(ret, nr + 2, alloc);
                ret[nr++] = t;
                str += len;

-- 
Regards,
Karthik Nayak