Michael,

On Wed, Jun 11, 2014 at 02:05:35PM +0200, Michael Haggerty wrote:
> 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.
> > 
...
> > 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';
> }
> 
grow_to() which could reduce in size, interesting.

> (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.
> 
I like your idea.  I am leaning towards doing the un-optimized
strbuf_set operations first, then optimizing in a later patch.

> > [...]
> 
> Michael
> 
> -- 
> Michael Haggerty
> mhag...@alum.mit.edu
> http://softwareswirl.blogspot.com/

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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

Reply via email to