Re: [PATCH] Rework strbuf API and semantics.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 2909 bytes --]

On Tue, Sep 04, 2007 at 11:11:45AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 4 Sep 2007, Pierre Habouzit wrote:
> 
> >   A strbuf can be used to store byte arrays, or as an extended string 
> > library. The `buf' member can be passed to any C legacy string function, 
> > because strbuf operations always ensure there is a terminating \0 at the 
> > end of the buffer, not accounted in the `len' field of the structure.
> > 
> >   A strbuf can be used to generate a string/buffer whose final size is 
> > not really known, and then "strbuf_detach" can be used to get the built 
> > buffer, and keep the wrapping "strbuf" structure usable for further work 
> > again.
> > 
> >   Other interesting feature: buffer_ensure(sb, size) ensure that there 
> > is enough allocated space in `sb' to put `size' new octets of data in 
> > the buffer. It helps avoiding reallocating data for nothing when the 
> > problem the strbuf helps to solve has a known typical size.
> 
> I like the general idea of this!
> 
> However, some comments are due:
> 
> - IMHO strbuf_grow() would be more descriptive than buffer_ensure(),

  Yeah, strbuf_ensure (and not buffer_ensure sorry :P) isn't a clever
name. strbuf_grow looks better indeed.

> - IMHO the same goes for strbuf_free() instead of strbuf_wipe(), and

  Well, I don't like strbuf_free, because it's opposed to
strbuf_alloc/new whatever, that would be functions that create and
release a struct strbuf * (meaning allocating the struct shell as well).
Here you only free the internal buffer, not the shell, so _free() would
be a very bad name.

  In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the
naming. Though, looking at git's code, it seems that the usual name for
this operation is _release. So would you go for strbuf_release ?


> - it would be nice to split this patch into
> 
> 	- the API change (with _minimal_ changes to anything outside of 
> 	  strbuf.[ch]), and
> 
> 	- the cleanups in the rest of the code.

  I'll try to do that, but it's quite hard to achieve knowing that in
many places of the current state of master, there are embeded NUL's
(accounted in ->len). I'll try to see what I can split, but it's
unlikely I'll be able to have an intermediate _working_ state (I mean
that would pass the testsuite) with the new API/semantics _and_ without
touching a lot less of the rest of the code.

  But I really understand that it helps seeing what the new API
improves, whereas right now it may not be that obvious because of the
(not so) many new lines in strbuf.[hc].


  I'll try to roll a new patchset ASAP.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]