On Mon, Aug 21, 2017 at 10:43 AM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:
<snip very well written commit message>
> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> - sb->buf[len] = '\0';
> + if (sb->buf != strbuf_slopbuf)
> + sb->buf[len] = '\0';
> + else
> + assert(!strbuf_slopbuf[0]);
> }
>
> /**
> --
> 2.14.1.151.gdfeca7a7e
>
I know this must have been discussed before and/or I'm having a neuron
misfire, but is there any reason why we didn't just make slopbuf a
macro for ""?
i.e.
#define strbuf_slopbuf ""
That way it should point to readonly memory and any attempt to assign
to it should produce a crash.
One benefit that I can think of for making strbuf_slopbuf be a real
variable is that we can then compare the pointer stored in the strbuf
to the strbuf_slopbuf address to detect whether the strbuf held the
slopbuf. With the static string macro, each execution unit may get
it's own instance of the empty string. But, before this patch, we
don't actually seem to be doing that anywhere and instead rely on the
value of alloc being accurate to determine whether the strbuf contains
the slopbuf or not.
So is there any reason why didn't do something like the following in
the first place?
diff --git a/strbuf.h b/strbuf.h
index e705b94..fcca618 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,7 +67,7 @@ struct strbuf {
char *buf;
};
-extern char strbuf_slopbuf[];
+#define strbuf_slopbuf ""
#define STRBUF_INIT { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
/**
@@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf
*sb, size_t len)
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
- sb->buf[len] = '\0';
+ if (sb->alloc) {
+ sb->buf[len] = '\0';
+ }
}
-Brandon
