I read the recent thread Timo Sirainen raised about string APIs in git. ANd I went read the strbuf.[hc] module. I believe that the choice made in that module are wrong and could be made better. I actually use to work with a string buffer API (that interested readers can look at on [0]), that work almost the same (except for the eof flag, but it's trivial to keep), but have two significant differences: First, and that's the most important one: the buffer is always NUL terminated, after its official "len". That means, in terms of strbuf API, that "alloc" is always greater or equal to len+1 to be able to store the ending NUL[1]. The advantages are obvious: you can pass the buffer to any legacy C string function without any fear of read overflow. strtol comes to mind, and atm, git has to explicitely use strbuf_end to put that ending nul to be able to call legacy applications. But once done, the NUL is accounted into the string (aka it's in "len") which makes it a non C-string (I mean you cannot append any more data in it anymore). So current implementations tries to workaround an issue (the non systematical NUL-termination) but IMHO the wrong way. The other shortcoming is that you cannot tell the buffer "Hey, it's very likely that you'll end up being _that_ long. That's why, in some parts of the code (see write_tar_entry in archive-tar.c e.g.) the programmer actually messes with the buffer allocation, outside from the strbuf module, which makes it well, useless. In my API, I have a "buffer_ensure" call, that is supposed to do that: "please ensure that this buffer still has _this_ amount of free and allocated space to put more data". So my question is, do people think I raise a valid point, and would patches that would refactor the strbuf module to have those functions, and would fix the code that uses strbuf's to interact properly, be accepted ? Also, the efficiency of the buffer module API I use has a lot to do with the fact that copying functions, and length tests are inlined in the .h, so that the compiler can optimize the ones it already tested 10 calls before. I'm not sure if this is frowned upon or if it makes sense. [0] http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.h;hb=HEAD http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.c;hb=HEAD [1] Of course, ensuring the NUL-termination has a cost, though it's often benign, and for performance-critical places where characters are copied one by one, it's always possible to use an "unsafe" addch (that would not maintain the invariant), and then call an equivalent of strbuf_end (that would not append a \0 like it does now, but just would fix the invariant that for any strbuf, buf->buf[buf->len] == '\0') explicitely. For places where the invariant generate negligible cost (like concatenating two paths parts with a middle '/' e.g.) then we gain safety without even having to think about it. -- ·O· Pierre Habouzit ··O madcoder@xxxxxxxxxx OOO http://www.madism.org
Attachment:
pgpoB5x81d23s.pgp
Description: PGP signature
