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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx
http://github.com/jmahler
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html