On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmah...@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).
It's also redundant to say "this patch" in the commit message, thus
should be avoided.
More below.
> Signed-off-by: Jeremiah Mahler <jmmah...@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 [...].
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.
> +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.
--
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
