Re: [PATCH] strbuf: allocate enough space when strbuf_setlen() is called first time

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 26.04.2011 23:51, schrieb Junio C Hamano:
> 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