Peff,
On Fri, Jun 13, 2014 at 03:15:50AM -0400, Jeff King wrote:
> On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:
>
> > > 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
Improving remote.c sounds like a better direction than adding set
operations. I will start looking in to it.
--
Jeremiah Mahler
jmmahler@xxxxxxxxx
http://github.com/jmahler
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html