Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
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
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

             reply index

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 ` 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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git