Pierre Habouzit [off-list ref] writes:
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: strbuf_grow(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.
"Rework API semantics" needs to be accompanied with an API
description, perhaps at the beginning of each externally
visible function.
Also the commit log message needs to explain what the old
semantics was and what the improved one is, to highlight the
changes needed to the callers. Especially...
quoted hunk
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)
if (term_len == command_buf.len
&& !strcmp(term, command_buf.buf))
break;
- ALLOC_GROW(buffer, length + command_buf.len, sz);
+ ALLOC_GROW(buffer, length + command_buf.len + 1, sz);
memcpy(buffer + length,
command_buf.buf,
- command_buf.len - 1);
- length += command_buf.len - 1;
+ command_buf.len);
+ length += command_buf.len;
buffer[length++] = '\n';
}
free(term);
... it is not all obvious why these off-by-one changes are
needed without such a description. The other hunks in this
patch to this file are all such changes.
-static void inline strbuf_add(struct strbuf *sb, int ch) {
+static inline void strbuf_addch(struct strbuf *sb, size_t c) {
+ strbuf_grow(sb, 1);
+ sb->buf[sb->len++] = c;
+ sb->buf[sb->len] = '\0';
+}
You certainly did not mean size_t wide characters.
On Thu, Sep 06, 2007 at 09:31:37AM +0000, Junio C Hamano wrote:
Pierre Habouzit [off-list ref] writes:
quoted
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: strbuf_grow(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.
"Rework API semantics" needs to be accompanied with an API
description, perhaps at the beginning of each externally
visible function.
Also the commit log message needs to explain what the old
semantics was and what the improved one is, to highlight the
changes needed to the callers. Especially...
quoted
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)
if (term_len == command_buf.len
&& !strcmp(term, command_buf.buf))
break;
- ALLOC_GROW(buffer, length + command_buf.len, sz);
+ ALLOC_GROW(buffer, length + command_buf.len + 1, sz);
memcpy(buffer + length,
command_buf.buf,
- command_buf.len - 1);
- length += command_buf.len - 1;
+ command_buf.len);
+ length += command_buf.len;
buffer[length++] = '\n';
}
free(term);
.... it is not all obvious why these off-by-one changes are
needed without such a description. The other hunks in this
patch to this file are all such changes.
Yes, as I suppose you know, but I state it here again so that
everybody understands, before strbuf's were merely a byte array, not
necessarily NUL-terminated. Hence many parts of the code that wanted to
pass the buffer to str* functions had to manually insert a NUL, hence it
was accounted in the length of the buffer.
Now, we always have a NUL after the "official" end of the buffer, so
it's not needed anymore. The off-by-ones are just that. The hunk you
quote is one where git's code was messing with strbufs internals
directly, so the ALLOC_GROW has to take the 1 octed needed to maintain
the internal invariant. Though, the patch after this one rewrites the
hunk to use strbuf's API's.
quoted
-static void inline strbuf_add(struct strbuf *sb, int ch) {
quoted
+static inline void strbuf_addch(struct strbuf *sb, size_t c) {
+ strbuf_grow(sb, 1);
+ sb->buf[sb->len++] = c;
+ sb->buf[sb->len] = '\0';
+}
You certainly did not mean size_t wide characters.
Oh boy, now I've been red-handed of :%s/\<int\>/\<size_t\>/ :)
I'll repost a _clean_ patch series soon with those things fixed, and
the wrong overflow test (that should be a >= and not a >) as well, as we
discussed it on IRC before.
Oh and FWIW I believe the details of the NUL always after the buffer
array has to be in strbuf.h and not in the commit comment, maybe I'll
put it in both to make everybody happy.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org