On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmah...@gmail.com> 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 dunno...
>> > Signed-off-by: Jeremiah Mahler <jmmah...@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
> jmmah...@gmail.com
> http://github.com/jmahler
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to