Re: [PATCH v3 1/2] add strbuf_set operations

From: Jeff King <peff@peff.net>
Date: Jun 13, 2014, 07:15 UTC
Message-ID: <20140613071550.GC7908@sigill.intra.peff.net>
In-Reply-To: <20140612234637.GB17803@hudson.localdomain>

On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:

Show 31 quoted lines

> >     Although strbuf_set() does make the code a bit easier to read
> >     when strbufs are repeatedly re-used, re-using a variable for
> >     different purposes is generally considered poor programming
> >     practice. It's likely that heavy re-use of strbufs has been
> >     tolerated to avoid multiple heap allocations, but that may be a
> >     case of premature (allocation) optimization, rather than good
> >     programming. A different ("better") way to make the code more
> >     readable and maintainable may be to ban re-use of strbufs for
> >     different purposes.
> > 
> > But I deleted it before sending because it's a somewhat tangential
> > issue not introduced by your changes. However, I do see strbuf_set()
> > as a Band-Aid for the problem described above, rather than as a useful
> > feature on its own. If the practice of re-using strbufs (as a
> > premature optimization) ever becomes taboo, then strbuf_set() loses
> > its value.
> > 
> 
> I am getting the feeling that I have mis-understood the purpose of
> strbufs.  It is not just a library to use in place of char*.
> 
> If strbufs should only be added to and never reset a good test would be
> to re-write builtin/remote.c without the use of strbuf_reset.
> 
> builtin/remote.c does re-use the buffers.  But it seems if a buffer is
> used N times then to avoid a reset you would need N buffers.
> 
> But on the other hand I agree with your comment that re-using a variable
> for different purposes is poor practice.
> 
> Now I am not even sure if I want my own patch :-)

I think reusing buffers like remote.c does makes things uglier and more confusing than necessary, and probably doesn't have any appreciable performance gain. We are saving a handful of allocations, and have such wonderful variable names as "buf2" (when is it OK to reuse that one, versus regular "buf"?).

A better reason I think is to reuse allocations across a loop, like:

  struct strbuf buf = STRBUF_INIT;

  for (i = 0; i < nr; i++) {
	strbuf_reset(&buf);
	strbuf_add(&buf, foo[i]);
	... do something with buf ...
  }
  strbuf_release(&buf);

You can write that as:

  for (i = 0; i < nr; i++) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_add(&buf, foo[i]);
	... do something ...
	strbuf_release(&buf);
  }

and it is definitely still a case of premature optimization. But:

  1. "nr" here may be very large, so the amortized benefits are greater

  2. It's only one call to strbuf_reset to cover many items. Not one
     sprinkled every few lines.

You'll note that strbuf_getline uses a "set" convention (making it different from the rest of strbuf) to handle this looping case.

I don't have a problem with strbuf_set, but just peeking at remote.c, I think I'd rather see it cleaned up. It looks like one of the major uses is setting config variables. I wonder how hard it would be to make a git_config_set variant that took printf-style formats, and handled the strbuf itself.

-Peff