Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions

Greetings Robear,

I don't have the expertise to talk about the larger picture here, but
I have the following suggestions if you do go with it.

I don't agree with this patchset being two different patches - The
changes in the second patch eliminate possibly buggy behavior of using
the new function.

I would also prefer the term "immutable" over "const" since const
already has implications in C programming.

STRBUF_INIT_CONST: a new way to initialize strbuf

Use imperative mood and be more specific in the commit title -
`strbuf: Teach strbuf to initialize immutable strings`

In a previous commit, a new function `STRBUF_INIT_CONST(const_str)`,
which would allow for the quick initialization of constant `strbuf`s,
was introduced.

Redundant if you merge both patches.

In this commit, I check through the strbuf_* functions to edit the ones
that would try to edit the passed `strbuf` so that they are guaranteed to
behave in a predictable way when met with a constant.

Added Functions:
  `strbuf_make_var`

Updated Functions:
  `strbuf_grow`
  `strbuf_setlen`
  `strbuf_trim`
   `strbuf_trim_trailing_dir_sep`
   `strbuf_trim_trailing_newline`
   `strbuf_tolower`
   `strbuf_splice`

Functions where comments were added to clarify the expected behavior:
   `strbuf_getcwd`

I feel this is self-explanatory when you go through the diff.

Signed-off-by: Robear Selwans <redacted>
---
 strbuf.c | 25 +++++++++++++++++++++++++
 strbuf.h | 14 ++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index f19da55b07..48af039b6e 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -89,6 +89,8 @@ void strbuf_attach(struct strbuf *sb, void *buf,

size_t len, size_t alloc)

 void strbuf_grow(struct strbuf *sb, size_t extra)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     int new_buf = !sb->alloc;
     if (unsigned_add_overflows(extra, 1) ||
         unsigned_add_overflows(sb->len, extra + 1))

@@ -100,8 +102,18 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
         sb->buf[0] = '\0';
 }

+void strbuf_make_var(struct strbuf *sb)
+{
+    char* str_cpy;
+    ALLOC_ARRAY(str_cpy, sb->len + 1);
+    memcpy(str_cpy, sb->buf, sb->len);
+    strbuf_attach(sb, str_cpy, sb->len, sb->len + 1);
+}
+
 void strbuf_trim(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     strbuf_rtrim(sb);
     strbuf_ltrim(sb);
 }

@@ -115,6 +127,8 @@ void strbuf_rtrim(struct strbuf *sb)

 void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
         sb->len--;
     sb->buf[sb->len] = '\0';

@@ -122,6 +136,9 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)

 void strbuf_trim_trailing_newline(struct strbuf *sb)
 {
+    if (sb->buf[sb->len - 1] == '\n')

+        if (sb->len > sb->alloc)
+            strbuf_make_var(sb);

Enclose this explicitly in braces.

     if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
         if (--sb->len > 0 && sb->buf[sb->len - 1] == '\r')
             --sb->len;

@@ -158,6 +175,8 @@ int strbuf_reencode(struct strbuf *sb, const char

*from, const char *to)

 void strbuf_tolower(struct strbuf *sb)
 {
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);
     char *p = sb->buf, *end = sb->buf + sb->len;
     for (; p < end; p++)
         *p = tolower(*p);

@@ -234,6 +253,8 @@ 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");
+    if (sb->len > sb->alloc)
+        strbuf_make_var(sb);

     if (dlen >= len)
         strbuf_grow(sb, dlen - len);

@@ -572,6 +593,10 @@ int strbuf_readlink(struct strbuf *sb, const char

*path, size_t hint)

 int strbuf_getcwd(struct strbuf *sb)
 {
+    /*
+     * If sb was a constant, then an error would lead to
+     * strbuf_release() being called on the sb.
+     */
     size_t oldalloc = sb->alloc;
     size_t guessed_len = 128;

diff --git a/strbuf.h b/strbuf.h
index 1a1753424c..a33bc4d90e 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -128,6 +128,10 @@ static inline void strbuf_swap(struct strbuf *a,

struct strbuf *b)
     SWAP(*a, *b);
 }

+/**
+ * Constant string buffer is turned into a variable one.
+ */
+void strbuf_make_var(struct strbuf *sb);

Use imperative mood - Convert an immutable string buffer to a variable
string buffer.

 /**
  * Functions related to the size of the buffer

@@ -160,6 +164,16 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
  */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {

+    if (sb->len > sb->alloc)
+    {
+        if (!len)
+        {
+            sb->buf = strbuf_slopbuf;
+            return;
+        }
+        else
+            strbuf_make_var(sb);
+    }

Use braces on the same line as the condition.

     if (len > (sb->alloc ? sb->alloc - 1 : 0))
         die("BUG: strbuf_setlen() beyond buffer");
     sb->len = len;
--
2.17.1