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@xxxxxxxxxx OOO http://www.madism.org
Attachment:
pgpAdOfJSVcmB.pgp
Description: PGP signature
