diff mbox series

[v2,12/14] builtin/patch-id: fix type of `get_one_patchid()`

Message ID 20241202-pks-sign-compare-v2-12-e7f0ad92a749@pks.im (mailing list archive)
State Superseded
Headers show
Series Start compiling with `-Wsign-compare` | expand

Commit Message

Patrick Steinhardt Dec. 2, 2024, 12:04 p.m. UTC
In `get_one_patchid()` we assign either the result of `strlen()` or
`remove_space()` to `len`. But while the former correctly returns a
`size_t`, the latter returns an `int` to indicate the length of the
string even though it cannot ever return a negative value. This causes a
warning with "-Wsign-conversion".

In fact, even `get_one_patchid()` itself is also using an integer as
return value even though it always returns the length of the patch, and
this bubbles up to other callers.

Adapt the function and its helpers to use `size_t` for string lengths
consistently.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/patch-id.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

shejialuo Dec. 2, 2024, 1:18 p.m. UTC | #1
On Mon, Dec 02, 2024 at 01:04:44PM +0100, Patrick Steinhardt wrote:
> In `get_one_patchid()` we assign either the result of `strlen()` or
> `remove_space()` to `len`. But while the former correctly returns a
> `size_t`, the latter returns an `int` to indicate the length of the

It is a little misleading about the statement "the latte returns an
`int` to indicate the length of the string".

Should "without spaces" be appended into the last? However, don't worth
a reroll.

> string even though it cannot ever return a negative value. This causes a
> warning with "-Wsign-conversion".
> 
> In fact, even `get_one_patchid()` itself is also using an integer as
> return value even though it always returns the length of the patch, and
> this bubbles up to other callers.
> 
> Adapt the function and its helpers to use `size_t` for string lengths
> consistently.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/patch-id.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 56585757477911c96bbb9ef2cf3710691b8e744e..f540d8daa736b027649c8c64ffe5100cf4044037 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -1,5 +1,4 @@
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "builtin.h"
>  #include "config.h"
> @@ -10,13 +9,13 @@
>  #include "parse-options.h"
>  #include "setup.h"
>  
> -static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
> +static void flush_current_id(size_t patchlen, struct object_id *id, struct object_id *result)
>  {
>  	if (patchlen)
>  		printf("%s %s\n", oid_to_hex(result), oid_to_hex(id));
>  }
>  
> -static int remove_space(char *line)
> +static size_t remove_space(char *line)

So, eventually, we have decided to change the return type from "int" to
"size_t". This avoids casting "int" to "size_t", which is good. And it
would be more natural, because "remove_space" never returns negative
result.

>  {
>  	char *src = line;
>  	char *dst = line;

Thanks,
Jialuo
Patrick Steinhardt Dec. 2, 2024, 1:24 p.m. UTC | #2
On Mon, Dec 02, 2024 at 09:18:35PM +0800, shejialuo wrote:
> On Mon, Dec 02, 2024 at 01:04:44PM +0100, Patrick Steinhardt wrote:
> > In `get_one_patchid()` we assign either the result of `strlen()` or
> > `remove_space()` to `len`. But while the former correctly returns a
> > `size_t`, the latter returns an `int` to indicate the length of the
> 
> It is a little misleading about the statement "the latte returns an
> `int` to indicate the length of the string".
> 
> Should "without spaces" be appended into the last? However, don't worth
> a reroll.

I'll say "stripped string" to keep things short. I won't send a reroll
just for this, but will send it out if there's going to be a v3 of this
series.

Thanks!

Patrick
diff mbox series

Patch

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 56585757477911c96bbb9ef2cf3710691b8e744e..f540d8daa736b027649c8c64ffe5100cf4044037 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,5 +1,4 @@ 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "config.h"
@@ -10,13 +9,13 @@ 
 #include "parse-options.h"
 #include "setup.h"
 
-static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
+static void flush_current_id(size_t patchlen, struct object_id *id, struct object_id *result)
 {
 	if (patchlen)
 		printf("%s %s\n", oid_to_hex(result), oid_to_hex(id));
 }
 
-static int remove_space(char *line)
+static size_t remove_space(char *line)
 {
 	char *src = line;
 	char *dst = line;
@@ -63,10 +62,11 @@  static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 	return 1;
 }
 
-static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
-			   struct strbuf *line_buf, int stable, int verbatim)
+static size_t get_one_patchid(struct object_id *next_oid, struct object_id *result,
+			      struct strbuf *line_buf, int stable, int verbatim)
 {
-	int patchlen = 0, found_next = 0;
+	size_t patchlen = 0;
+	int found_next = 0;
 	int before = -1, after = -1;
 	int diff_is_binary = 0;
 	char pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];
@@ -78,7 +78,7 @@  static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
 	while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
 		char *line = line_buf->buf;
 		const char *p = line;
-		int len;
+		size_t len;
 
 		/* Possibly skip over the prefix added by "log" or "format-patch" */
 		if (!skip_prefix(line, "commit ", &p) &&
@@ -179,7 +179,7 @@  static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
 static void generate_id_list(int stable, int verbatim)
 {
 	struct object_id oid, n, result;
-	int patchlen;
+	size_t patchlen;
 	struct strbuf line_buf = STRBUF_INIT;
 
 	oidclr(&oid, the_repository->hash_algo);