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

[Jeremiah Mahler <jmmahler@gmail.com>]

Junio,

Comments below...

On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > A common use case with strubfs is to set the buffer to a new value.
> > This must be done in two steps: a reset followed by an add.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, new_buf, len);
> >
> > In cases where the buffer is being built up in steps, these operations
> > make sense and correctly convey what is being performed.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, data1, len1);
> >   strbuf_add(buf, data2, len2);
> >   strbuf_add(buf, data3, len3);
> >
> > However, in other cases, it can be confusing and is not very concise.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, default, len1);
> >
> >   if (cond1) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data3, len3);
> >   }
> >
> > Add strbuf_set operations so that it can be re-written in a clear and
> > concise way.
> >
> >   strbuf_set(buf, default len1);
> >
> >   if (cond1) {
> >     strbuf_set(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_set(buf, data3, len3);
> >   }
> 
> Or even more concisely without making unnecessary internal calls to
> strbuf_reset():
> 
> 	strbuf_reset(buf);
>         if (cond2)
>         	strbuf_add(buf, data3, len3);
> 	else if (cond1)
>         	strbuf_add(buf, data2, len2);
> 	else
>         	strbuf_add(buf, default, len2);
> 
> ;-)

_add would work in your example.  strbuf_set would also work and it
would perform the same number of operations (one set and one add).

However your example is different from mine in which cond1 and cond2 are
not necessarily mutually exclusive.

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

> 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