Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time
- From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
- Date
- Apr 27, 2011, 17:24 UTC
- Message-ID
- <4DB85162.6000204@lsrfire.ath.cx>
- In-Reply-To
- <7vhb9kd6kp.fsf@alter.siamese.dyndns.org>
Am 26.04.2011 23:51, schrieb Junio C Hamano:
Show 22 quoted lines
> René Scharfe<rene.scharfe@lsrfire.ath.cx> writes:
>
>> How about something like this instead? The call to strbuf_grow() was
>> introduced in a8f3e2219 when there was no strbuf_slopbuf buffer that
>> nowadays makes sure we always have a place to write an initial NUL.
>> We can take it out again now, simplifying the code and hopefully
>> avoiding future confusion.
>
> Thanks; I think that makes sense.
>
> It further may make sense to turn the assert into BUG() though, to clarify
> what kind of programming error we are trying to catch. Perhaps like:
>
>> static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
>> + assert(len< (sb->alloc ? sb->alloc : 1));
>
> if (len< (sb->alloc ? sb->alloc : 1))
> die("programming error: using strbuf_setlen() to extend a strbuf");
>
>> sb->len = len;
>> sb->buf[len] = '\0';
>> }I like the idea, except the comparison needs to be inverted.
Compiled, but not tested. The test suite takes too long and skips too many tests on my Windows box and I don't have any other machine available right now. :-/
-- >8 -- Subject: strbuf: clarify assertion in strbuf_setlen()
Commit a8f3e2219 introduced the strbuf_grow() call to strbuf_setlen() to make ensure that there was at least one byte available to write the mandatory trailing NUL, even for previously unallocated strbufs.
Then b315c5c0 added strbuf_slopbuf for the same reason, only globally for all uses of strbufs.
Thus the strbuf_grow() call can be removed now. This avoids readers of strbuf.h from mistakenly thinking that strbuf_setlen() can be used to extend a strbuf.
The following assert() needs to be changed to cope with the fact that sb->alloc can now be zero, which is OK as long as len is also zero. As suggested by Junio, use the chance to convert it to a die() with a short explanatory message. The pattern of 'die("BUG: ...")' is already used in strbuf.c.
This was the only assert() in strbuf.[ch], so assert.h doesn't have to be included anymore either.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx> --- strbuf.h | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/strbuf.h b/strbuf.h index 07060ce..9e6d9fa 100644 --- a/strbuf.h +++ b/strbuf.h @@ -3,8 +3,6 @@ /* See Documentation/technical/api-strbuf.txt */ -#include <assert.h> - extern char strbuf_slopbuf[]; struct strbuf { size_t alloc; @@ -33,9 +31,8 @@ static inline size_t strbuf_avail(const struct strbuf *sb) { extern void strbuf_grow(struct strbuf *, size_t); static inline void strbuf_setlen(struct strbuf *sb, size_t len) { - if (!sb->alloc) - strbuf_grow(sb, 0); - assert(len < sb->alloc); + if (len > (sb->alloc ? sb->alloc - 1 : 0)) + die("BUG: strbuf_setlen() beyond buffer"); sb->len = len; sb->buf[len] = '\0'; }
-- 1.7.5