Re: Use of strbuf.buf when strbuf.len == 0

From: Pierre Habouzit <madcoder@debian.org>
Date: Sep 29, 2007, 07:48 UTC
Message-ID: <20070929074813.GA3366@artemis.corp>
In-Reply-To: <alpine.LFD.0.999.0709281746500.3579@woody.linux-foundation.org>

On Sat, Sep 29, 2007 at 12:51:36AM +0000, Linus Torvalds wrote:

Show 29 quoted lines

> 
> 
> On Thu, 27 Sep 2007, Pierre Habouzit wrote:
> > 
> >   I can see a way, that would need special proof-reading of the strbuf
> > module, but should not harm its users, that would be to change
> > STRBUF_INIT to work this way:
> > 
> >   { .buf = "", .len = 0, .alloc = 0 }
> 
> I'd like to pipe up a bit here..
> 
> I think the above is a good fix for the current problem of wanting to 
> always be able to use "sb->buf", but I thinkit actually has the potential 
> to fix another issue entirely.
> 
> Namely strbuf's that are initialized from various static strings and/or 
> strings not directly allocated with malloc().
> 
> That's not necessarily something really unusual. Wanting to initialize a 
> string with a fixed constant value is a common problem.
> 
> And wouldn't it be nice if you could actually do that, with
> 
> 	{ .buf = "static initializer", .len = 18, .alloc = 0 }
> 
> and have all the strbuf routines that modify the initializer (including 
> making it shorter!) notice that the allocation is too short, and create a 
> new allocation?

  We could probably do that. The places to change to make this work are
seldom:
  * strbuf_grow to emulate a realloc (and copy the buffer into the new
    malloc()ed one),
  * strbuf_release assumes that ->alloc == 0 means _init isn't
    necessary, it would be now,
  * strbuf_setlen should not have the assert anymore (though I'm not
    sure this assert still makes sense with the new initializer).
and that's it.

  But we cannot initialize a strbuf with an immediate string because all
the strbuf APIs suppose that the strbuf buffer are writeable (and IMHO
it's pointless to use a strbuf for reading purposes). Other point, I've
made many readings of the code searching for specific patterns of code,
to replace with strbuf's, and I've never seen places (I do not say those
don't exists though) that would directly benefit from that.

> Hmm?

  So I'd say I'll keep the idea in mind, because it's tasteful and could
help, though I'd prefer Junio to review that patch, and then later add
this new semantics if the need arises.

  The sole thing that may be worth investigating would look like:

      char internal_buf[PATH_MAX]; /* should be damn long enough */
      struct strbuf buf;

      strbuf_init_static(&buf, internal_buf, sizeof(internal_buf);

      /* hack with the strbuf API */

      strbuf_release(&buf); /* do release memory, in case we went over
                               PATH_MAX octets */

because it could save some allocations _and_ be safe at the same time. But I don't really like it, when allocation is critical in git, it's rarely in functions where there is an obvious size limit for the problem, and avoiding allocations can be done using static strbufs (fast-import.c does that in many places).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org