Eric,
On Thu, Jun 12, 2014 at 05:48:52PM -0400, Eric Sunshine wrote:
> On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@xxxxxxxxx> wrote:
> > On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
> >> I am on the fence.
> >>
> >> I have this suspicion that the addition of strbuf_set() would *only*
> >> help when the original written with reset-and-then-add sequence was
> >> suboptimal to begin with, and it helps *only* how the code reads,
> >> without correcting the fact that it is still doing unnecessary
> >> "first set to a value to be discarded and then reset to set the
> >> right value", sweeping the issue under the rug.
> >>
> > It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> > benefit from this operation because it is so badly designed. But this
> > seems unlikely given the review process around here ;-)
> >
> > The one case where it would be doing extra work is when a strbuf_set
> > replaces a strbuf_add that didn't previously have a strbuf_reset.
> > strbuf_set is not appropriate for all cases, as I mentioned in the
> > patch, but in some cases I think it makes it more readable. And in this
> > case it would be doing a reset on an empty strbuf. Is avoiding this
> > expense worth the reduction in readability?
> >
> > Also, as Eric Sunshine pointed out, being able to easily re-order
> > operations can make the code easier to maintain.
> >
> >> Repeated reset-and-then-add on the same strbuf used to be something
> >> that may indicate that the code is doing unnecessary work. Now,
> >> repeated uses of strbuf_set on the same strbuf replaced that pattern
> >> to be watched for to spot wasteful code paths.
> >>
> > If a reset followed by and add was a rare occurrence I would tend to
> > agree more.
>
> When composing my review of the builtin/remote.c changes, I wrote
> something like this:
>
> 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 dunno...
> >>
> >> > Signed-off-by: Jeremiah Mahler <jmmahler@xxxxxxxxx>
> >> > ---
> >> > Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >> > strbuf.c | 21 +++++++++++++++++++++
> >> > strbuf.h | 13 +++++++++++++
> >> > 3 files changed, 52 insertions(+)
> >> >
> >> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> >> > index f9c06a7..ae9c9cc 100644
> >> > --- a/Documentation/technical/api-strbuf.txt
> >> > +++ b/Documentation/technical/api-strbuf.txt
> >> > @@ -149,6 +149,24 @@ Functions
> >> > than zero if the first buffer is found, respectively, to be less than,
> >> > to match, or be greater than the second buffer.
> >> > /*----- add data in your buffer -----*/
> > ...
> >> > static inline void strbuf_addch(struct strbuf *sb, int c)
> >> > {
> >
> > --
> > Jeremiah Mahler
> > jmmahler@xxxxxxxxx
> > http://github.com/jmahler
--
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