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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <robear.selwans@xxxxxxxxxxx>
---
 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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux