Re: [PATCH] use strbuf_addbuf() for appending a strbuf to another

From: Jeff King <peff@peff.net>
Date: Jul 21, 2016, 21:02 UTC
Message-ID: <20160721210253.GA4604@sigill.intra.peff.net>
In-Reply-To: <5790FC74.6000602@web.de>

On Thu, Jul 21, 2016 at 06:46:44PM +0200, René Scharfe wrote:

Show 12 quoted lines

> >> -	strbuf_add(out, untracked->ident.buf, untracked->ident.len);
> >> +	strbuf_addbuf(out, &untracked->ident);
> > 
> > This is actually slightly _less_ efficient, because we already are using
> > the precomputed len, and the new code will call an extra strbuf_grow()
> > to cover the case where the two arguments are the same.  See 81d2cae
> > (strbuf_addbuf(): allow passing the same buf to dst and src,
> > 2010-01-12).
> 
> Ah, I wasn't aware of that.  Calling strbuf_grow() twice shouldn't be
> thaaat bad.  However, I wonder where we duplicate strbufs, or why we
> would ever want to do so.  Anyway, here's a patch for that:

I don't think we do. Going back to the original discussion:

  http://thread.gmane.org/gmane.comp.version-control.git/136141/focus=136774

it was mostly just "hey, this would fail really confusingly if we ever did, so let's make it safe".

The second strbuf_grow() is by definition a noop (which is why 81d2cae works at all), but we do pay the size-computation cost.

Show 6 quoted lines

> -- >8 --
> Subject: strbuf: avoid calling strbuf_grow() twice in strbuf_addbuf()
> 
> Implement strbuf_addbuf() as a normal function in order to avoid calling
> strbuf_grow() twice, with the second callinside strbud_add() being a
> no-op.  This is slightly faster and also reduces the text size a bit.

Seems reasonable. IMHO the main advantage is that one does not have to reason about the double strbuf_grow() (i.e., that the strbuf_add() is safe because we know its grow is a noop).

-Peff