On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
>
>
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >
> > - strbuf_grow(buf, len);
> > + /* only grow if not in place */
> > + if (strbuf_avail(buf) + buf->len < len)
> > + strbuf_grow(buf, len - buf->len);
>
> Umm. This is really ugly.
>
> The whole point of strbuf's was that you shouldn't be doing your own
> allocation decisions etc. So why do it?
>
> Wouldn't it be much better to have a strbuf_make_room() interface that
> just guarantees that there is enough room fo "len"?
>
> Otherwise, code like the above would seem to make the whole point of a
> safer string interface rather pointless. The above code only makes sense
> if you know how the strbuf's are internally done, so it should not exists
> except as internal strbuf code. No?
Took a short look at strbuf.h after seeing the above code.
And I was suprised to see that all strbuf users were exposed to
the strbuf structure.
Following patch would at least make sure noone fiddle with strbuf internals.
Cut'n'paste - only for the example of it.
It simply moves strbuf declaration to the .c file where it rightfully belongs.
git did not build with this change....
Sam
diff --git a/strbuf.c b/strbuf.c
index e33d06b..0d2d578 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,14 @@
#include "cache.h"
#include "strbuf.h"
+struct strbuf {
+ int alloc;
+ int len;
+ int eof;
+ char *buf;
+};
+
+
void strbuf_init(struct strbuf *sb) {
sb->buf = NULL;
sb->eof = sb->alloc = sb->len = 0;
diff --git a/strbuf.h b/strbuf.h
index 74cc012..c057be3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,11 +1,6 @@
#ifndef STRBUF_H
#define STRBUF_H
-struct strbuf {
- int alloc;
- int len;
- int eof;
- char *buf;
-};
+struct strbuf;
extern void strbuf_init(struct strbuf *);
extern void read_line(struct strbuf *, FILE *, int);
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
