On Mon, Feb 26, 2024 at 5:39 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Achu Luma <ach.lumap@xxxxxxxxx> 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.
> > 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.
> > + 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.
> > + 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);
> > +}
