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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 12, 2014, 21:48 UTC
Message-ID: <CAPig+cTVLJQOsW7H4Ht2NNYkeiMb=EWT7BG3sNu0wNsTQ=oZNA@mail.gmail.com>
In-Reply-To: <20140612193144.GA17077@hudson.localdomain>

On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:

Show 31 quoted lines

> 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.

Show 25 quoted lines

>> I dunno...
>>
>> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
>> > ---
>> >  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@gmail.com
> http://github.com/jmahler