{"thread":{"id":"54933","subject":"[RFC PATCH v1 0/1] strbuf.c/h: add the constant version initialization method of strbuf","startedAt":"2021-01-05T06:44:41Z","lastAt":"2021-01-07T07:04:51Z","messageCount":3,"participants":["ZheNing Hu","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"413435","messageId":"20210105064502.725307-1-adlternative@gmail.com","threadId":"54933","inReplyTo":null,"subject":"[RFC PATCH v1 0/1] strbuf.c/h: add the constant version initialization method of strbuf","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-01-05T06:45:01Z","receivedAt":"2021-01-05T06:44:41Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"According to\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet/,\nwe can create a new way to initialize strbuf\nwith string constantto save the overhead of\ndynamically allocated memory.The marco function\nSTRBUF_INIT_CONST(str).For example,STRBUF_ININ_DEFAULT(\"default\")\nwill create a strbuf which buf eqoal to \"default\",\nlen eqoal to 7 and alloc eqoal to 0.This constant\nstrbuf will be changed to dynamic allocation when\nwe want to modify the contents of the constant buffer,\nThe function strbuf_const_to_no_const do this by change\nbuf with xstrdup,some function like strbuf_grow and\nstrbuf_setlen will use it,to change the strbuf\nto a dynamically expand buffer.\n\nMy inference is based on an non-const strbuf\nwith alloc greater than 0 and len greater than 0,\nconst strbuf with alloc = 0 and len greater than 0.\nBut now it have some bug because of I don't know where\nis the situation that non-const strbuf with 0 alloc\nand more then 0 len,this is a bit out of my expectation.\nSo I fail check in gitgitgadget's check.\nWho can give me some advice?\n\nThanks.\n\nZheNing Hu (1):\n  strbuf.c/h: add the constant version initialization method of strbuf\n\n strbuf.c | 33 +++++++++++++++++++++++++++++----\n strbuf.h |  8 ++++++++\n 2 files changed, 37 insertions(+), 4 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"413436","messageId":"20210105064502.725307-2-adlternative@gmail.com","threadId":"54933","inReplyTo":"20210105064502.725307-1-adlternative@gmail.com","subject":"[RFC PATCH v1 1/1] strbuf.c/h: add the constant version initialization method of strbuf","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-01-05T06:45:02Z","receivedAt":"2021-01-05T06:44:58Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"\nSigned-off-by: ZheNing Hu <adlternative@gmail.com>\n---\n strbuf.c | 33 +++++++++++++++++++++++++++++----\n strbuf.h |  8 ++++++++\n 2 files changed, 37 insertions(+), 4 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex e3397cc4c7..6e1fd2e628 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -58,17 +58,32 @@ void strbuf_init(struct strbuf *sb, size_t hint)\n \t\tstrbuf_grow(sb, hint);\n }\n \n+void strbuf_const_to_no_const(struct strbuf *sb)\n+{\n+\tif (sb->len && !sb->alloc) {\n+\t\tchar *new_buf = xstrdup(sb->buf);\n+\t\tint len = sb->len;\n+\t\tstrbuf_init(sb, sb->len);\n+\t\tsb->buf = new_buf;\n+\t\tsb->len = len;\n+\t\tsb->buf[sb->len] = '\\0';\n+\t}\n+}\n void strbuf_release(struct strbuf *sb)\n {\n \tif (sb->alloc) {\n \t\tfree(sb->buf);\n \t\tstrbuf_init(sb, 0);\n-\t}\n+\t}else if(sb->len)\n+\t\tstrbuf_init(sb, 0);\n }\n \n char *strbuf_detach(struct strbuf *sb, size_t *sz)\n {\n \tchar *res;\n+\tif (sb->len && !sb->alloc)\n+    \t\tdie(\"you should not use detach in a const_strbuf\");\n+\n \tstrbuf_grow(sb, 0);\n \tres = sb->buf;\n \tif (sz)\n@@ -89,7 +104,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n \n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n-\tint new_buf = !sb->alloc;\n+\tint new_buf;\n+\tstrbuf_const_to_no_const(sb);\n+\tnew_buf = !sb->alloc;\n \tif (unsigned_add_overflows(extra, 1) ||\n \t    unsigned_add_overflows(sb->len, extra + 1))\n \t\tdie(\"you want to use way too much memory\");\n@@ -108,6 +125,7 @@ void strbuf_trim(struct strbuf *sb)\n \n void strbuf_rtrim(struct strbuf *sb)\n {\n+\tstrbuf_const_to_no_const(sb);\n \twhile (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))\n \t\tsb->len--;\n \tsb->buf[sb->len] = '\\0';\n@@ -115,6 +133,7 @@ void strbuf_rtrim(struct strbuf *sb)\n \n void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n {\n+\tstrbuf_const_to_no_const(sb);\n \twhile (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))\n \t\tsb->len--;\n \tsb->buf[sb->len] = '\\0';\n@@ -122,6 +141,7 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n \n void strbuf_trim_trailing_newline(struct strbuf *sb)\n {\n+\tstrbuf_const_to_no_const(sb);\n \tif (sb->len > 0 && sb->buf[sb->len - 1] == '\\n') {\n \t\tif (--sb->len > 0 && sb->buf[sb->len - 1] == '\\r')\n \t\t\t--sb->len;\n@@ -131,7 +151,9 @@ void strbuf_trim_trailing_newline(struct strbuf *sb)\n \n void strbuf_ltrim(struct strbuf *sb)\n {\n-\tchar *b = sb->buf;\n+\tchar *b;\n+\tstrbuf_const_to_no_const(sb);\n+\tb = sb->buf;\n \twhile (sb->len > 0 && isspace(*b)) {\n \t\tb++;\n \t\tsb->len--;\n@@ -158,7 +180,9 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n \n void strbuf_tolower(struct strbuf *sb)\n {\n-\tchar *p = sb->buf, *end = sb->buf + sb->len;\n+\tchar *p,*end;\n+\tstrbuf_const_to_no_const(sb);\n+\tp = sb->buf, end = sb->buf + sb->len;\n \tfor (; p < end; p++)\n \t\t*p = tolower(*p);\n }\n@@ -234,6 +258,7 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n \t\tdie(\"`pos' is too far after the end of the buffer\");\n \tif (pos + len > sb->len)\n \t\tdie(\"`pos + len' is too far after the end of the buffer\");\n+\tstrbuf_const_to_no_const(sb);\n \n \tif (dlen >= len)\n \t\tstrbuf_grow(sb, dlen - len);\ndiff --git a/strbuf.h b/strbuf.h\nindex 223ee2094a..0bfab0177d 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -72,6 +72,13 @@ struct strbuf {\n extern char strbuf_slopbuf[];\n #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n \n+#define STRBUF_INIT_CONST(str)  { .alloc = 0, .len = strlen(str), .buf = str }\n+\n+/*\n+ *  Through this function, we can turn a constant buffer into a non-constant buffer\n+ */\n+void strbuf_const_to_no_const(struct strbuf *sb);\n+\n /*\n  * Predeclare this here, since cache.h includes this file before it defines the\n  * struct.\n@@ -159,6 +166,7 @@ void strbuf_grow(struct strbuf *sb, size_t amount);\n  */\n static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n {\n+\tstrbuf_const_to_no_const(sb);\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n \t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n \tsb->len = len;\n-- \n2.30.0\n\n"},{"id":"413658","messageId":"CAPig+cQL=b-nF6nADaWueJaDmxCgmZbUwWj6=dAwYQ=vVrkifg@mail.gmail.com","threadId":"54933","inReplyTo":"20210105064502.725307-2-adlternative@gmail.com","subject":"Re: [RFC PATCH v1 1/1] strbuf.c/h: add the constant version initialization method of strbuf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-07T07:03:41Z","receivedAt":"2021-01-07T07:04:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2021 at 1:46 AM ZheNing Hu <adlternative@gmail.com> wrote:\n> Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n\nPlease write a commit message which (at least briefly) explains why\nthis change is useful.\n\n> diff --git a/strbuf.c b/strbuf.c\n> @@ -58,17 +58,32 @@ void strbuf_init(struct strbuf *sb, size_t hint)\n> +void strbuf_const_to_no_const(struct strbuf *sb)\n> +{\n> +       if (sb->len && !sb->alloc) {\n> +               char *new_buf = xstrdup(sb->buf);\n\nstrbuf is allowed to contain '\\0' characters, so this call to\nxstrdup() will not allocate the correct amount of memory if there is\nan embedded '\\0'\n\n> +               int len = sb->len;\n> +               strbuf_init(sb, sb->len);\n> +               sb->buf = new_buf;\n> +               sb->len = len;\n> +               sb->buf[sb->len] = '\\0';\n> +       }\n> +}\n\nThis function can probably be simplified to:\n\n    void strbuf_const_to_no_const(struct strbuf *sb)\n    {\n        if (sb->len && !sb->alloc) {\n            const char *v = sb->buf;\n            size_t n = sb->len;\n            strbuf_init(sb, n);\n            strbuf_add(sb, v, n);\n        }\n    }\n\n>  void strbuf_release(struct strbuf *sb)\n>  {\n>         if (sb->alloc) {\n>                 free(sb->buf);\n>                 strbuf_init(sb, 0);\n> -       }\n> +       }else if(sb->len)\n> +               strbuf_init(sb, 0);\n>  }\n\nI think this can be simplified to:\n\n    void strbuf_release(struct strbuf *sb)\n    {\n        if (sb->alloc)\n            free(sb->buf);\n        if (sb->len)\n            strbuf_init(sb, 0);\n    }\n\nBut it's probably okay to simplify it even further:\n\n    void strbuf_release(struct strbuf *sb)\n    {\n        if (sb->alloc)\n            free(sb->buf);\n        strbuf_init(sb, 0);\n    }\n\n>  char *strbuf_detach(struct strbuf *sb, size_t *sz)\n>  {\n>         char *res;\n> +       if (sb->len && !sb->alloc)\n> +               die(\"you should not use detach in a const_strbuf\");\n\nI can't think of a good reason to enforce this harsh restriction. This\npatch updates all the other destructive functions so they work\ncorrectly with a buffer which was initialized from a constant string,\nso this function should be able to do the same. For instance, I\nbelieve the following would work instead:\n\n    if (sb->len && !sb->alloc)\n        strbuf_const_to_no_const(sb);\n\n>         strbuf_grow(sb, 0);\n\nIn fact, since you changed strbuf_grow() to convert the buffer from\nconst to non-const, then you should be able to remove the above\nconditional and die() altogether.\n\n> @@ -89,7 +104,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n>  void strbuf_grow(struct strbuf *sb, size_t extra)\n>  {\n> -       int new_buf = !sb->alloc;\n> +       int new_buf;\n> +       strbuf_const_to_no_const(sb);\n> +       new_buf = !sb->alloc;\n> diff --git a/strbuf.h b/strbuf.h\n> @@ -72,6 +72,13 @@ struct strbuf {\n>  extern char strbuf_slopbuf[];\n>  #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n> +#define STRBUF_INIT_CONST(str)  { .alloc = 0, .len = strlen(str), .buf = str }\n\nThere is a fundamental problem here. If the programmer writes:\n\n    static struct strbuf x = STRBUF_INIT_CONST(\"\");\n\nthen both `len` and `alloc` will be zero, so the conditional you use elsewhere:\n\n    if (sb->len && !sb->alloc)\n\nwill not be able to detect that `buf` is pointing at a constant\nstring. You _may_ be able to work around this problem like this:\n\n    if (!sb->alloc && (sb->len || sb->buf != strbuf_slopbuf))\n\nto accurately detect a strbuf initialized with a constant string (but\nI haven't tested this). Or, it might be possible to simplify it\nfurther to:\n\n    if (!sb->alloc && sb->buf != strbuf_slopbuf)\n\nIt would be a good idea to add a new (private) function which\nencapsulates the complex condition so that it doesn't have to be\nrepeated all over the place. Perhaps:\n\n    static int is_const(struct strbuf *sb) {\n        return !sb->alloc && sb->buf != strbuf_slopbuf;\n    }\n\nor something.\n\n> +/*\n> + *  Through this function, we can turn a constant buffer into a non-constant buffer\n> + */\n> +void strbuf_const_to_no_const(struct strbuf *sb);\n\n\"constant\" strbufs are an implementation detail which we probably\nwouldn't want to publish as part of the public API. Unfortunately,\nthis function is needed by inline strbuf_setlen(), which is why you\nadded it to the header. Even so, because this is an implementation\ndetail, we may want to warn people against calling this function.\nPerhaps like this:\n\n    /* private -- do not call */\n    void strbuf_const_to_no_const(struct strbuf *sb);\n\n> @@ -159,6 +166,7 @@ void strbuf_grow(struct strbuf *sb, size_t amount);\n>  static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n>  {\n> +       strbuf_const_to_no_const(sb);\n>         if (len > (sb->alloc ? sb->alloc - 1 : 0))\n>                 die(\"BUG: strbuf_setlen() beyond buffer\");\n>         sb->len = len;\n\nIn [1], Dscho suggested that if the requested `len` is zero, then it\ncould treat that case specially by setting `buf` to `strbuf_slopbuf`\nrather than going through the wasteful work of calling\nstrbuf_const_to_no_const(). Doing so may require moving the suggested\nis_const() to the header, as well, so:\n\n    /* private -- do not call */\n    int strbuf_is_const(struct strbuf *sb);\n\n[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet/\n"}]}