Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 26, 2024, 17:15 UTC
Message-ID: <CAP8UFD0Qhy78=v9+hCekMJPkcH2KmeZeQ0xUx8kqrByQ4PO3Xg@mail.gmail.com>
In-Reply-To: <xmqqil2bdvsy.fsf@gitster.g>

On Mon, Feb 26, 2024 at 5:39 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

>
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In a following commit we are going to port code from
> > "t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
> > a new "t/unit-tests/t-hash.c" file using the recently added unit test
> > framework.
> >
> > To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
> > we are going to need a new strbuf_addstrings() function that repeatedly
> > adds the same string a number of times to a buffer.
>
> We do not need to call such a function "addstrings", though.  The
> name on the subject line made me expect a varargs function:
>
>  (bad)  strbuf_addstrings(&sb, "foo", "bar", "baz", NULL);
>
> It would have been clearer if the name hinted what it does, clearer
> than just a single "s" that says it is talking about plural.  What
> would be a good name that hints "n times add a single same string"?
> I dunno.
>
> I also would have expected that the order of parameters are
> repeat-count followed by what gets repeated.
>
> Having said all of the above, we already have "addchars" that is
> equally strange, so let's let it pass ;-).

Yeah, we thought about naming it strbuf_repeatstr() first, but then seeing addchars() I thought it would be better to imitate it.

Show 14 quoted lines

> > diff --git a/strbuf.c b/strbuf.c
> > index 7827178d8e..eb2b3299ce 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
> >       strbuf_setlen(sb, sb->len + len);
> >  }
> >
> > +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
> > +{
> > +     size_t len = strlen(s);
>
> Let's have a blank line here to separate decls from the first
> statement.

Yeah, right.

Show 9 quoted lines

> > +     if (unsigned_mult_overflows(len, n))
> > +             die("you want to use way too much memory");
> > +     strbuf_grow(sb, len * n);
>
> The error message given by
>
>         strbuf_grow(sb, st_mult(len, n));
>
> would be equally informative and takes only a single line.

It seems that the pattern in strbuf.c, for example in strbuf_splice() and strbuf_vinsertf(), is to do a check first using an *_overflows() function, die if the check fails, and only then call strbuf_grow(). So we did the same. I am fine with using your suggestion though.

Show 6 quoted lines

> > +     for (size_t i = 0; i < n; i++)
> > +             memcpy(sb->buf + sb->len + len * i, s, len);
>
> Wouldn't it be sufficient to run strbuf_add() n times at this point,
> as we have already called strbuf_grow() to avoid repeated
> reallocation?

Unfortunately strbuf_add() calls strbuf_grow() itself which is not needed as we have already called strbuf_grow() to avoid repeated reallocation.

>  Repeated manual memcpy() that involves manual offset
> computation makes me nervous.

I would have prefered to avoid it too, but didn't find a good way to do it.

> > +     strbuf_setlen(sb, sb->len + len * n);
> > +}