sideband.c: replace int with size_t for clarity

From: Chandra Pratap <[email protected]>

Replace int with size_t for clarity and remove the 'NEEDSWORK' tag associated with it.

Signed-off-by Chandra Pratap <[email protected]>

---
sideband.c: replace int with size_t for clarity

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1625

 sideband.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

sideband.c +0 -0

index 6cbfd391c47..1599e408d1b 100644

Quoted 21 lines · Chandra Pratap via GitGitGadget

From: Chandra Pratap <[email protected]>

Replace int with size_t for clarity and remove the 'NEEDSWORK' tag associated with it.

Signed-off-by Chandra Pratap <[email protected]>

---
sideband.c: replace int with size_t for clarity

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1625

 sideband.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

sideband.c +0 -0

index 6cbfd391c47..1599e408d1b 100644

sideband.c void list_config_color_sideband_slots(struct string_list *list, const char *pref · line 76

Quoted 10 lines

sideband.c +1 -2

@@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * of the line. This should be called for a single line only, which is
  * passed as the first N characters of the SRC array.
  *
- * NEEDSWORK: use "size_t n" instead for clarity.
  */
-static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)
 {
      int i;
 

Thanks for working on this.
There is, however, more potential for improvements. First of all: the "int i" from above. Further down, we read

        for (i = 0; i < ARRAY_SIZE(keywords); i++) {

However, a size of an array can never be negative, so that an unsigned data type is a better choice than a signed. And, arrays can have more elements than an int can address, at least in theory.
For a reader it makes more sense, to replace
int i;
with
size_t i;

And further down, there is another place for improvments:

                int len = strlen(p->keyword);
                if (n < len)
                        continue;

Even here, a strlen is never negative. And a size_t is the choice for len, since all "modern" implementations declare strlen() to return size_t

Do you think that you can have a look at these changes ?

I will be happy to do a review, and possibly other people as well.

Thanks for the feedback, Torsten. I was working on improving the rest of sideband.c as you suggested when I encountered this snippet on line 82:

while (0 < n && isspace(*src)) {
strbuf_addch(dest, *src);
src++;
n--;
}

Here, we are decreasing the value of an unsigned type to potentially below 0 which may lead to overflow and result in some nasty bug. In this case, is it wise to continue with replacing 'int n' with 'size_t n' as the NEEDSWORK tag suggests or should we improve upon the rest of the file and revert 'size_t n' to 'int n' ?

Quoted 62 lines · Chandra Pratap via GitGitGadget

Quoted 21 lines

From: Chandra Pratap <[email protected]>

Replace int with size_t for clarity and remove the 'NEEDSWORK' tag associated with it.

Signed-off-by Chandra Pratap <[email protected]>

---
sideband.c: replace int with size_t for clarity

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1625

 sideband.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

sideband.c +0 -0

index 6cbfd391c47..1599e408d1b 100644

sideband.c void list_config_color_sideband_slots(struct string_list *list, const char *pref · line 76

Quoted 10 lines

sideband.c +1 -2

@@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * of the line. This should be called for a single line only, which is
  * passed as the first N characters of the SRC array.
  *
- * NEEDSWORK: use "size_t n" instead for clarity.
  */
-static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)
 {
      int i;
 

Thanks for working on this.
There is, however, more potential for improvements. First of all: the "int i" from above. Further down, we read

        for (i = 0; i < ARRAY_SIZE(keywords); i++) {

However, a size of an array can never be negative, so that an unsigned data type is a better choice than a signed. And, arrays can have more elements than an int can address, at least in theory.
For a reader it makes more sense, to replace
int i;
with
size_t i;

And further down, there is another place for improvments:

                int len = strlen(p->keyword);
                if (n < len)
                        continue;

Even here, a strlen is never negative. And a size_t is the choice for len, since all "modern" implementations declare strlen() to return size_t

Do you think that you can have a look at these changes ?

I will be happy to do a review, and possibly other people as well.

Further down, we read

	for (i = 0; i < ARRAY_SIZE(keywords); i++) {

However, a size of an array can never be negative, so that an unsigned data type is a better choice than a signed. And, arrays can have more elements than an int can address, at least in theory.
For a reader it makes more sense, to replace
int i;
with
size_t i;

Torsten Bögershausen <[email protected]> writes:

Just this part.

Quoted 11 lines

Further down, we read

	for (i = 0; i < ARRAY_SIZE(keywords); i++) {

However, a size of an array can never be negative, so that an unsigned data type is a better choice than a signed. And, arrays can have more elements than an int can address, at least in theory.
For a reader it makes more sense, to replace
int i;
with
size_t i;

It is a very good discipline to use size_t to index into an array whose size is externally controled (e.g., we slurp what the end user or the server on the other end of the connection gave us into a piece of memory we allocate) to avoid integer overflows as "int" is often narrower than "size_t". But this particular one is a Meh; the keywords[] is a small hardcoded array whose size and contents are totally under our control.

- * NEEDSWORK: use "size_t n" instead for clarity.
++ * It is fine to use "int n" here instead of "size_t n" as all calls to this ++ * function pass an 'int' parameter.

Subject: Re: [PATCH v4] sideband.c: remove redundant 'NEEDSWORK' tag

From: Chandra Pratap <[email protected]>

Signed-off-by Chandra Pratap <[email protected]>

---
sideband.c: replace int with size_t for clarity

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1625

Range-diff vs v3:

1: 273415aa6a4 ! 1: 8c003256e5b sideband.c: remove redundant 'NEEDSWORK' tag
@@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
*
- * NEEDSWORK: use "size_t n" instead for clarity.
+ * It is fine to use "int n" here instead of "size_t n" as all calls to this
-+ * function pass an 'int' parameter.
++ * function pass an 'int' parameter. Additionally, the buffer involved in ++ * storing these 'int' values takes input from a packet via the pkt-line ++ * interface, which is capable of transferring only 64kB at a time.
*/
static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) {

 sideband.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

sideband.c +4 -1

index 6cbfd391c47..266a67342be 100644
@@ -69,7 +69,10 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
  * of the line. This should be called for a single line only, which is
  * passed as the first N characters of the SRC array.
  *
- * NEEDSWORK: use "size_t n" instead for clarity.
+ * It is fine to use "int n" here instead of "size_t n" as all calls to this
+ * function pass an 'int' parameter. Additionally, the buffer involved in
+ * storing these 'int' values takes input from a packet via the pkt-line
+ * interface, which is capable of transferring only 64kB at a time.
  */
 static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 {

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996