Message ID | 20210105064502.725307-2-adlternative@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | strbuf.c/h: add the constant version initialization method of strbuf | expand |
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/
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;
Signed-off-by: ZheNing Hu <adlternative@gmail.com> --- strbuf.c | 33 +++++++++++++++++++++++++++++---- strbuf.h | 8 ++++++++ 2 files changed, 37 insertions(+), 4 deletions(-)