On 06/10/2014 12:19 AM, Jeremiah Mahler 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);
>
> 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.
> +
> +`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);
> +}
> +
I never know how much intelligence to attribute to modern compilers, but
it seems like even after optimization this function will be doing more
work than necessary:
strbuf_reset(sb)
-> strbuf_setlen(sb, 0)
-> sb->len = 0
-> sb->buf[len] = '\0'
-> strbuf_add(sb, data, len)
-> strbuf_grow(sb, len)
-> ...lots of stuff...
-> memcpy(sb->buf + sb->len, data, len)
-> strbuf_setlen(sb, sb->len + len)
-> sb->len = len
-> sb->buf[len] = '\0'
If there were a function like strbuf_grow_to(sb, len):
void strbuf_grow_to(struct strbuf *sb, size_t len)
{
int new_buf = !sb->alloc;
if (unsigned_add_overflows(len, 1))
die("you want to use way too much memory");
if (new_buf)
sb->buf = NULL;
ALLOC_GROW(sb->buf, len + 1, sb->alloc);
if (new_buf)
sb->buf[0] = '\0';
}
(strbuf_grow() could call it:
static inline void strbuf_grow(struct strbuf *sb, size_t extra)
{
if (unsigned_add_overflows(sb->len, extra))
die("you want to use way too much memory");
strbuf_grow_to(sb, sb->len + extra);
}
), then your function could be minimized to
void strbuf_set(struct strbuf *sb, const void *data, size_t len)
{
strbuf_grow_to(sb, len);
memcpy(sb->buf, data, len);
strbuf_setlen(sb, len);
}
I think strbuf_grow_to() would be useful in other situations too.
This is just an idea; I'm not saying that you have to make this change.
> [...]
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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