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

On Wed, Jun 11, 2014 at 04:42:56AM -0400, Eric Sunshine wrote:

On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Jeremiah Mahler <redacted>
---
 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.

quoted

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

quoted

+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