Re: [PATCH v2 01/19] add strbuf_set operations

[Jeremiah Mahler <jmmahler@gmail.com>]

On Wed, Jun 11, 2014 at 04:42:56AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > Currently, the data in a strbuf is modified using add operations.  To
> > set the buffer to some data a reset must be performed before an add.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, cb.buf.buf, cb.buf.len);
> >
> > And this is a common sequence of operations with 70 occurrences found in
> > the current source code.  This includes all the different variations
> > (add, addf, addstr, addbuf, addch).
> >
> >   FILES=`find ./ -name '*.c'`
> >   CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
> >   CNT=$(echo "$CNT / 2" | bc)
> >   echo $CNT
> >   70
> >
> > These patches add strbuf_set operations which allow this common sequence
> > to be performed in one line instead of two.
> >
> >   strbuf_set(buf, cb.buf.buf, cb.buf.len);
> 
> This commit message is effectively the cover letter for the entire
> patch series; it doesn't say specifically what _this_ patch is doing.
> 
> Justification for the change could be stronger. Rather than merely
> pointing out that a sequence of operations occurs N times in the
> project, explain why strbuf_set() is superior. For instance, you might
> say something about how strbuf_set() conveys the operation being
> performed more concisely and clearly than strbuf_reset() +
> strbuf_add() (and thus may reduce cognitive load, though that's
> subjective). The bit about performing the operation in one line
> instead of two is minor, at best, and may not be worth mentioning at
> all (since it's implied).
> 
After reviewing my argument, I think the biggest benefit is that it can
make the code more readable in some cases.

> It's also redundant to say "this patch" in the commit message, thus
> should be avoided.
Got it.

> 
> More below.
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >  strbuf.c                               | 21 +++++++++++++++++++++
> >  strbuf.h                               | 14 ++++++++++++++
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> > index 077a709..b7e23da 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.
> >
> > +* Setting the buffer
> > +
> > +`strbuf_set`::
> > +
> > +       Replace the buffer content with data of a given length.
> 
> I know that you copied the wording I suggested in my v1 review, but
> now that I see it in context, I find the redundancy level rather high.
> The header already says "Setting the buffer", so repeating "the
> buffer" in each function description doesn't add much. It might make
> sense to reword as:
> 
>     Replace content with [...].
> 
Fixed.

> More below.
> 
> > +`strbuf_setstr`::
> > +
> > +       Replace the buffer content with data from a NUL-terminated string.
> > +
> > +`strbuf_setf`::
> > +
> > +       Replace the buffer content with a formatted string.
> > +
> > +`strbuf_setbuf`::
> > +
> > +       Replace the buffer content with data from another buffer.
> > +
> >  * Adding data to the buffer
> >
> >  NOTE: All of the functions in this section will grow the buffer as necessary.
> > diff --git a/strbuf.c b/strbuf.c
> > index ac62982..9d64b00 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
> >         strbuf_setlen(sb, sb->len + dlen - len);
> >  }
> >
> > +void strbuf_set(struct strbuf *sb, const void *data, size_t len)
> > +{
> > +       strbuf_reset(sb);
> > +       strbuf_add(sb, data, len);
> > +}
> > +
> > +void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
> > +{
> > +       va_list ap;
> > +       strbuf_reset(sb);
> > +       va_start(ap, fmt);
> > +       strbuf_vaddf(sb, fmt, ap);
> > +       va_end(ap);
> > +}
> > +
> > +void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
> > +{
> > +       strbuf_reset(sb);
> > +       strbuf_add(sb, sb2->buf, sb2->len);
> > +}
> > +
> >  void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
> >  {
> >         strbuf_splice(sb, pos, 0, data, len);
> > diff --git a/strbuf.h b/strbuf.h
> > index e9ad03e..b339f08 100644
> > --- a/strbuf.h
> > +++ b/strbuf.h
> > @@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
> >   */
> >  extern void strbuf_list_free(struct strbuf **);
> >
> > +/*----- set buffer to data -----*/
> > +
> 
> Minor: Existing divider lines in this header are not followed by a blank line.
Fixed.
> 
> > +extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
> > +
> > +static inline void strbuf_setstr(struct strbuf *sb, const char *s)
> > +{
> > +       strbuf_set(sb, s, strlen(s));
> > +}
> > +
> > +__attribute__((format (printf,2,3)))
> > +extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
> > +
> > +extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
> > +
> >  /*----- add data in your buffer -----*/
> >  static inline void strbuf_addch(struct strbuf *sb, int c)
> >  {
> > --
> > 2.0.0.592.gf55b190
> 
> Final word: The comments in this review do not necessarily require a
> re-roll. Junio may or may not want to pick up the series as is. If he
> doesn't, or if you want to polish it further, then perhaps take the
> review comments into consideration when rerolling.

They are certainly changes worth making.
Thanks again for your review :-)

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler