[RFC PATCH v1 0/1] strbuf.c/h: add the constant version initialization method of strbuf

[RFC PATCH v1 0/1] strbuf.c/h: add the constant version initialization method of strbuf

[ZheNing Hu]

According to
https://public-inbox.org/git/nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet/,
we can create a new way to initialize strbuf
with string constantto save the overhead of
dynamically allocated memory.The marco function
STRBUF_INIT_CONST(str).For example,STRBUF_ININ_DEFAULT("default")
will create a strbuf which buf eqoal to "default",
len eqoal to 7 and alloc eqoal to 0.This constant
strbuf will be changed to dynamic allocation when
we want to modify the contents of the constant buffer,
The function strbuf_const_to_no_const do this by change
buf with xstrdup,some function like strbuf_grow and
strbuf_setlen will use it,to change the strbuf
to a dynamically expand buffer.

My inference is based on an non-const strbuf
with alloc greater than 0 and len greater than 0,
const strbuf with alloc = 0 and len greater than 0.
But now it have some bug because of I don't know where
is the situation that non-const strbuf with 0 alloc
and more then 0 len,this is a bit out of my expectation.
So I fail check in gitgitgadget's check.
Who can give me some advice?

Thanks.

ZheNing Hu (1):
  strbuf.c/h: add the constant version initialization method of strbuf

 strbuf.c | 33 +++++++++++++++++++++++++++++----
 strbuf.h |  8 ++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.30.0

[RFC PATCH v1 1/1] strbuf.c/h: add the constant version initialization method of strbuf

[ZheNing Hu]

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 strbuf.c | 33 +++++++++++++++++++++++++++++----
 strbuf.h |  8 ++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index e3397cc4c7..6e1fd2e628 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -58,17 +58,32 @@ void strbuf_init(struct strbuf *sb, size_t hint)
 		strbuf_grow(sb, hint);
 }
 
+void strbuf_const_to_no_const(struct strbuf *sb)
+{
+	if (sb->len && !sb->alloc) {
+		char *new_buf = xstrdup(sb->buf);
+		int len = sb->len;
+		strbuf_init(sb, sb->len);
+		sb->buf = new_buf;
+		sb->len = len;
+		sb->buf[sb->len] = '\0';
+	}
+}
 void strbuf_release(struct strbuf *sb)
 {
 	if (sb->alloc) {
 		free(sb->buf);
 		strbuf_init(sb, 0);
-	}
+	}else if(sb->len)
+		strbuf_init(sb, 0);
 }
 
 char *strbuf_detach(struct strbuf *sb, size_t *sz)
 {
 	char *res;
+	if (sb->len && !sb->alloc)
+    		die("you should not use detach in a const_strbuf");
+
 	strbuf_grow(sb, 0);
 	res = sb->buf;
 	if (sz)
@@ -89,7 +104,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
 
 void strbuf_grow(struct strbuf *sb, size_t extra)
 {
-	int new_buf = !sb->alloc;
+	int new_buf;
+	strbuf_const_to_no_const(sb);
+	new_buf = !sb->alloc;
 	if (unsigned_add_overflows(extra, 1) ||
 	    unsigned_add_overflows(sb->len, extra + 1))
 		die("you want to use way too much memory");
@@ -108,6 +125,7 @@ void strbuf_trim(struct strbuf *sb)
 
 void strbuf_rtrim(struct strbuf *sb)
 {
+	strbuf_const_to_no_const(sb);
 	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
 	sb->buf[sb->len] = '\0';
@@ -115,6 +133,7 @@ void strbuf_rtrim(struct strbuf *sb)
 
 void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 {
+	strbuf_const_to_no_const(sb);
 	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
 	sb->buf[sb->len] = '\0';
@@ -122,6 +141,7 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 
 void strbuf_trim_trailing_newline(struct strbuf *sb)
 {
+	strbuf_const_to_no_const(sb);
 	if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
 		if (--sb->len > 0 && sb->buf[sb->len - 1] == '\r')
 			--sb->len;
@@ -131,7 +151,9 @@ void strbuf_trim_trailing_newline(struct strbuf *sb)
 
 void strbuf_ltrim(struct strbuf *sb)
 {
-	char *b = sb->buf;
+	char *b;
+	strbuf_const_to_no_const(sb);
+	b = sb->buf;
 	while (sb->len > 0 && isspace(*b)) {
 		b++;
 		sb->len--;
@@ -158,7 +180,9 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
 
 void strbuf_tolower(struct strbuf *sb)
 {
-	char *p = sb->buf, *end = sb->buf + sb->len;
+	char *p,*end;
+	strbuf_const_to_no_const(sb);
+	p = sb->buf, end = sb->buf + sb->len;
 	for (; p < end; p++)
 		*p = tolower(*p);
 }
@@ -234,6 +258,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 		die("`pos' is too far after the end of the buffer");
 	if (pos + len > sb->len)
 		die("`pos + len' is too far after the end of the buffer");
+	strbuf_const_to_no_const(sb);
 
 	if (dlen >= len)
 		strbuf_grow(sb, dlen - len);
diff --git a/strbuf.h b/strbuf.h
index 223ee2094a..0bfab0177d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -72,6 +72,13 @@ struct strbuf {
 extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
 
+#define STRBUF_INIT_CONST(str)  { .alloc = 0, .len = strlen(str), .buf = str }
+
+/*
+ *  Through this function, we can turn a constant buffer into a non-constant buffer
+ */
+void strbuf_const_to_no_const(struct strbuf *sb);
+
 /*
  * Predeclare this here, since cache.h includes this file before it defines the
  * struct.
@@ -159,6 +166,7 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
  */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
+	strbuf_const_to_no_const(sb);
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
 		die("BUG: strbuf_setlen() beyond buffer");
 	sb->len = len;
-- 
2.30.0

Re: [RFC PATCH v1 1/1] strbuf.c/h: add the constant version initialization method of strbuf

[Eric Sunshine]

On Tue, Jan 5, 2021 at 1:46 AM ZheNing Hu <adlternative@gmail.com> wrote:
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

Please write a commit message which (at least briefly) explains why
this change is useful.

> diff --git a/strbuf.c b/strbuf.c
> @@ -58,17 +58,32 @@ void strbuf_init(struct strbuf *sb, size_t hint)
> +void strbuf_const_to_no_const(struct strbuf *sb)
> +{
> +       if (sb->len && !sb->alloc) {
> +               char *new_buf = xstrdup(sb->buf);

strbuf is allowed to contain '\0' characters, so this call to
xstrdup() will not allocate the correct amount of memory if there is
an embedded '\0'

> +               int len = sb->len;
> +               strbuf_init(sb, sb->len);
> +               sb->buf = new_buf;
> +               sb->len = len;
> +               sb->buf[sb->len] = '\0';
> +       }
> +}

This function can probably be simplified to:

    void strbuf_const_to_no_const(struct strbuf *sb)
    {
        if (sb->len && !sb->alloc) {
            const char *v = sb->buf;
            size_t n = sb->len;
            strbuf_init(sb, n);
            strbuf_add(sb, v, n);
        }
    }

>  void strbuf_release(struct strbuf *sb)
>  {
>         if (sb->alloc) {
>                 free(sb->buf);
>                 strbuf_init(sb, 0);
> -       }
> +       }else if(sb->len)
> +               strbuf_init(sb, 0);
>  }

I think this can be simplified to:

    void strbuf_release(struct strbuf *sb)
    {
        if (sb->alloc)
            free(sb->buf);
        if (sb->len)
            strbuf_init(sb, 0);
    }

But it's probably okay to simplify it even further:

    void strbuf_release(struct strbuf *sb)
    {
        if (sb->alloc)
            free(sb->buf);
        strbuf_init(sb, 0);
    }

>  char *strbuf_detach(struct strbuf *sb, size_t *sz)
>  {
>         char *res;
> +       if (sb->len && !sb->alloc)
> +               die("you should not use detach in a const_strbuf");

I can't think of a good reason to enforce this harsh restriction. This
patch updates all the other destructive functions so they work
correctly with a buffer which was initialized from a constant string,
so this function should be able to do the same. For instance, I
believe the following would work instead:

    if (sb->len && !sb->alloc)
        strbuf_const_to_no_const(sb);

>         strbuf_grow(sb, 0);

In fact, since you changed strbuf_grow() to convert the buffer from
const to non-const, then you should be able to remove the above
conditional and die() altogether.

> @@ -89,7 +104,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
>  void strbuf_grow(struct strbuf *sb, size_t extra)
>  {
> -       int new_buf = !sb->alloc;
> +       int new_buf;
> +       strbuf_const_to_no_const(sb);
> +       new_buf = !sb->alloc;
> diff --git a/strbuf.h b/strbuf.h
> @@ -72,6 +72,13 @@ struct strbuf {
>  extern char strbuf_slopbuf[];
>  #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
> +#define STRBUF_INIT_CONST(str)  { .alloc = 0, .len = strlen(str), .buf = str }

There is a fundamental problem here. If the programmer writes:

    static struct strbuf x = STRBUF_INIT_CONST("");

then both `len` and `alloc` will be zero, so the conditional you use elsewhere:

    if (sb->len && !sb->alloc)

will not be able to detect that `buf` is pointing at a constant
string. You _may_ be able to work around this problem like this:

    if (!sb->alloc && (sb->len || sb->buf != strbuf_slopbuf))

to accurately detect a strbuf initialized with a constant string (but
I haven't tested this). Or, it might be possible to simplify it
further to:

    if (!sb->alloc && sb->buf != strbuf_slopbuf)

It would be a good idea to add a new (private) function which
encapsulates the complex condition so that it doesn't have to be
repeated all over the place. Perhaps:

    static int is_const(struct strbuf *sb) {
        return !sb->alloc && sb->buf != strbuf_slopbuf;
    }

or something.

> +/*
> + *  Through this function, we can turn a constant buffer into a non-constant buffer
> + */
> +void strbuf_const_to_no_const(struct strbuf *sb);

"constant" strbufs are an implementation detail which we probably
wouldn't want to publish as part of the public API. Unfortunately,
this function is needed by inline strbuf_setlen(), which is why you
added it to the header. Even so, because this is an implementation
detail, we may want to warn people against calling this function.
Perhaps like this:

    /* private -- do not call */
    void strbuf_const_to_no_const(struct strbuf *sb);

> @@ -159,6 +166,7 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
>  static inline void strbuf_setlen(struct strbuf *sb, size_t len)
>  {
> +       strbuf_const_to_no_const(sb);
>         if (len > (sb->alloc ? sb->alloc - 1 : 0))
>                 die("BUG: strbuf_setlen() beyond buffer");
>         sb->len = len;

In [1], Dscho suggested that if the requested `len` is zero, then it
could treat that case specially by setting `buf` to `strbuf_slopbuf`
rather than going through the wasteful work of calling
strbuf_const_to_no_const(). Doing so may require moving the suggested
is_const() to the header, as well, so:

    /* private -- do not call */
    int strbuf_is_const(struct strbuf *sb);

[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet/