From: Abhishek Kumar <abhishekkumar8222@gmail.com>
To: rwagih.rw@gmail.com
Cc: git@vger.kernel.org, gitster@pobox.com, l.s.r@web.de,
pclouds@gmail.com, peff@peff.net, predatoramigo@gmail.com
Subject: Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions
Date: Tue, 18 Feb 2020 15:00:00 +0530 [thread overview]
Message-ID: <CAHk66fskrfcJ0YFDhfimVBTJZB4um7r=GdQuM8heJdZtF8D7UQ@mail.gmail.com> (raw)
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@outlook.com>
---
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
next reply other threads:[~2020-02-18 9:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-18 9:30 Abhishek Kumar [this message]
2020-02-18 14:42 ` [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions Robear Selwans
2020-02-18 20:46 ` Junio C Hamano
2020-02-19 1:43 ` Robear Selwans
2020-02-19 2:05 ` Jeff King
2020-02-19 3:13 ` Junio C Hamano
2020-02-19 4:34 ` Robear Selwans
2020-02-19 10:44 ` Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2020-02-18 4:18 [GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover Robear Selwans
2020-02-18 4:18 ` [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions Robear Selwans
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk66fskrfcJ0YFDhfimVBTJZB4um7r=GdQuM8heJdZtF8D7UQ@mail.gmail.com' \
--to=abhishekkumar8222@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=predatoramigo@gmail.com \
--cc=rwagih.rw@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).