{"thread":{"id":"62902","subject":"[GSOC][PATCH] apply: address -Wsign-comparison warnings","startedAt":"2025-02-05T01:42:42Z","lastAt":"2025-02-25T12:53:31Z","messageCount":31,"participants":["Zejun Zhao","Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511842","messageId":"20250205014055.737190-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":null,"subject":"[GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-05T01:40:55Z","receivedAt":"2025-02-05T01:42:42Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"There are several -Wsign-comparison warnings in \"apply.c\", which can be\nclassified into the following three types:\n\n 1. comparing a `length` of `size_t` type with a result of pointer\n arithmetic of `int` type\n\n 2. comparing a `length` of `size_t` type with a `length` of `int` type\n\n 3. comparing a loop count `i` of `int` type with an unsigned loop\n bound\n\nFix these warnings following one basic principle: do not touch the\nrelevant logics and keep the behaviors of the code. Adopt three\ndifferent strategies for each of the above three types:\n\n 1. cast the result of pointer arithmetic to `size_t` type\n\n 2. try to change the type of the `length` to `size_t` (may fall back\n to Strategy 1 if the variable is not guaranteed to be unsigned)\n\n 3. use a loop count `i` of `size_t` type\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 73 +++++++++++++++++++++++++++------------------------------\n apply.h | 6 ++---\n 2 files changed, 38 insertions(+), 41 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4a7b6120ac..0c7b89dc3a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -8,7 +8,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n@@ -540,7 +539,7 @@ static size_t date_len(const char *line, size_t len)\n \t !isdigit(*p++) || !isdigit(*p++))\t/* Not a date. */\n \t\treturn 0;\n \n-\tif (date - line >= strlen(\"19\") &&\n+\tif ((size_t) (date - line) >= strlen(\"19\") &&\n \t isdigit(date[-1]) && isdigit(date[-2]))\t/* 4-digit year */\n \t\tdate -= strlen(\"19\");\n \n@@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,\n \t * and optional space with octal mode.\n \t */\n \tconst char *ptr, *eol;\n-\tint len;\n-\tconst unsigned hexsz = the_hash_algo->hexsz;\n+\tsize_t len;\n+\tconst size_t hexsz = the_hash_algo->hexsz;\n \n \tptr = strchr(line, '.');\n-\tif (!ptr || ptr[1] != '.' || hexsz < ptr - line)\n+\tif (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))\n \t\treturn 0;\n \tlen = ptr - line;\n \tmemcpy(patch->old_oid_prefix, line, len);\n@@ -1158,7 +1157,7 @@ static const char *skip_tree_prefix(int p_value,\n */\n static char *git_header_name(int p_value,\n \t\t\t const char *line,\n-\t\t\t int llen)\n+\t\t\t size_t llen)\n {\n \tconst char *name;\n \tconst char *second = NULL;\n@@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value,\n \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n \t\tif (!cp)\n \t\t\tgoto free_and_fail1;\n-\t\tif (line + llen - cp != first.len ||\n+\t\tif ((size_t) (line + llen - cp) != first.len ||\n \t\t memcmp(first.buf, cp, first.len))\n \t\t\tgoto free_and_fail1;\n \t\treturn strbuf_detach(&first, NULL);\n@@ -1240,7 +1239,7 @@ static char *git_header_name(int p_value,\n \t\t\t\tgoto free_and_fail2;\n \n \t\t\tlen = sp.buf + sp.len - np;\n-\t\t\tif (len < second - name &&\n+\t\t\tif (len < (size_t) (second - name) &&\n \t\t\t !strncmp(np, name, len) &&\n \t\t\t isspace(name[len])) {\n \t\t\t\t/* Good */\n@@ -1317,7 +1316,7 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t int *linenr,\n \t\t\t int p_value,\n \t\t\t const char *line,\n-\t\t\t int len,\n+\t\t\t size_t len,\n \t\t\t unsigned int size,\n \t\t\t struct patch *patch)\n {\n@@ -1371,14 +1370,13 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t{ \"index \", gitdiff_index },\n \t\t\t{ \"\", gitdiff_unrecognized },\n \t\t};\n-\t\tint i;\n \n \t\tlen = linelen(line, size);\n \t\tif (!len || line[len-1] != '\\n')\n \t\t\tbreak;\n-\t\tfor (i = 0; i < ARRAY_SIZE(optable); i++) {\n+\t\tfor (size_t i = 0; i < ARRAY_SIZE(optable); i++) {\n \t\t\tconst struct opentry *p = optable + i;\n-\t\t\tint oplen = strlen(p->str);\n+\t\t\tsize_t oplen = strlen(p->str);\n \t\t\tint res;\n \t\t\tif (len < oplen || memcmp(p->str, line, oplen))\n \t\t\t\tcontinue;\n@@ -1592,7 +1590,7 @@ static int find_header(struct apply_state *state,\n \t\t\t\t\t\t\t\tsize, patch);\n \t\t\tif (git_hdr_len < 0)\n \t\t\t\treturn -128;\n-\t\t\tif (git_hdr_len <= len)\n+\t\t\tif ((size_t) git_hdr_len <= len)\n \t\t\t\tcontinue;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\n@@ -2097,7 +2095,6 @@ static void add_name_limit(struct apply_state *state,\n static int use_patch(struct apply_state *state, struct patch *p)\n {\n \tconst char *pathname = p->new_name ? p->new_name : p->old_name;\n-\tint i;\n \n \t/* Paths outside are not touched regardless of \"--include\" */\n \tif (state->prefix && *state->prefix) {\n@@ -2107,7 +2104,7 @@ static int use_patch(struct apply_state *state, struct patch *p)\n \t}\n \n \t/* See if it matches any of exclude/include rule */\n-\tfor (i = 0; i < state->limit_by_name.nr; i++) {\n+\tfor (size_t i = 0; i < state->limit_by_name.nr; i++) {\n \t\tstruct string_list_item *it = &state->limit_by_name.items[i];\n \t\tif (!wildmatch(it->string, pathname, 0))\n \t\t\treturn (it->util != NULL);\n@@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t};\n \t\t\tint i;\n \t\t\tfor (i = 0; binhdr[i]; i++) {\n-\t\t\t\tint len = strlen(binhdr[i]);\n+\t\t\t\tsize_t len = strlen(binhdr[i]);\n \t\t\t\tif (len < size - hd &&\n \t\t\t\t !memcmp(binhdr[i], buffer + hd, len)) {\n \t\t\t\t\tstate->linenr++;\n@@ -2238,7 +2235,8 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n {\n \tstruct strbuf qname = STRBUF_INIT;\n \tchar *cp = patch->new_name ? patch->new_name : patch->old_name;\n-\tint max, add, del;\n+\tsize_t max;\n+\tint add, del;\n \n \tquote_c_style(cp, &qname, NULL, 0);\n \n@@ -2257,12 +2255,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n \t}\n \n \tif (patch->is_binary) {\n-\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n+\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n \t\tstrbuf_release(&qname);\n \t\treturn;\n \t}\n \n-\tprintf(\" %-*s |\", max, qname.buf);\n+\tprintf(\" %-*s |\", (int) max, qname.buf);\n \tstrbuf_release(&qname);\n \n \t/*\n@@ -2319,7 +2317,7 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tstruct image fixed_preimage = IMAGE_INIT;\n \tsize_t insert_pos = 0;\n-\tint i, ctx, reduced;\n+\tsize_t ctx, reduced;\n \tconst char *fixed;\n \n \t/*\n@@ -2328,7 +2326,7 @@ static void update_pre_post_images(struct image *preimage,\n \t * free \"oldlines\".\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n-\tfor (i = 0; i < fixed_preimage.line_nr; i++)\n+\tfor (size_t i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n \timage_clear(preimage);\n \t*preimage = fixed_preimage;\n@@ -2337,7 +2335,7 @@ static void update_pre_post_images(struct image *preimage,\n \t/*\n \t * Adjust the common context lines in postimage.\n \t */\n-\tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n+\tfor (size_t i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n \n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n@@ -2417,9 +2415,9 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t struct image *postimage,\n \t\t\t\t unsigned long current,\n \t\t\t\t int current_lno,\n-\t\t\t\t int preimage_limit)\n+\t\t\t\t size_t preimage_limit)\n {\n-\tint i;\n+\tsize_t i;\n \tsize_t imgoff = 0;\n \tsize_t preoff = 0;\n \tsize_t extra_chars;\n@@ -2486,12 +2484,12 @@ static int match_fragment(struct apply_state *state,\n \t\t\t unsigned ws_rule,\n \t\t\t int match_beginning, int match_end)\n {\n-\tint i;\n+\tsize_t i;\n \tconst char *orig, *target;\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n \tsize_t fixed_len;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \tint ret;\n \n \tif (preimage->line_nr + current_lno <= img->line_nr) {\n@@ -2663,12 +2661,11 @@ static int match_fragment(struct apply_state *state,\n \tfor ( ; i < preimage->line_nr; i++) {\n \t\tsize_t fixstart = fixed.len; /* start of the fixed preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n-\t\tint j;\n \n \t\t/* Try fixing the line in the preimage */\n \t\tws_fix_copy(&fixed, orig, oldlen, ws_rule, NULL);\n \n-\t\tfor (j = fixstart; j < fixed.len; j++) {\n+\t\tfor (size_t j = fixstart; j < fixed.len; j++) {\n \t\t\tif (!isspace(fixed.buf[j])) {\n \t\t\t\tret = 0;\n \t\t\t\tgoto out;\n@@ -2705,7 +2702,7 @@ static int find_pos(struct apply_state *state,\n {\n \tint i;\n \tunsigned long backwards, forwards, current;\n-\tint backwards_lno, forwards_lno, current_lno;\n+\tsize_t backwards_lno, forwards_lno, current_lno;\n \n \t/*\n \t * When running with --allow-overlap, it is possible that a hunk is\n@@ -2790,7 +2787,7 @@ static int find_pos(struct apply_state *state,\n */\n static void update_image(struct apply_state *state,\n \t\t\t struct image *img,\n-\t\t\t int applied_pos,\n+\t\t\t size_t applied_pos,\n \t\t\t struct image *preimage,\n \t\t\t struct image *postimage)\n {\n@@ -2798,11 +2795,11 @@ static void update_image(struct apply_state *state,\n \t * remove the copy of preimage at offset in img\n \t * and replace it with postimage\n \t */\n-\tint i, nr;\n+\tint nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tsize_t result_alloc;\n \tchar *result;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \n \t/*\n \t * If we are removing blank lines at the end of img,\n@@ -2817,11 +2814,11 @@ static void update_image(struct apply_state *state,\n \tif (preimage_limit > img->line_nr - applied_pos)\n \t\tpreimage_limit = img->line_nr - applied_pos;\n \n-\tfor (i = 0; i < applied_pos; i++)\n+\tfor (size_t i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n \n \tremove_count = 0;\n-\tfor (i = 0; i < preimage_limit; i++)\n+\tfor (size_t i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n \tinsert_count = postimage->buf.len;\n \n@@ -2850,7 +2847,7 @@ static void update_image(struct apply_state *state,\n \t\t\t img->line_nr - (applied_pos + preimage_limit));\n \tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);\n \tif (!state->allow_overlap)\n-\t\tfor (i = 0; i < postimage->line_nr; i++)\n+\t\tfor (size_t i = 0; i < postimage->line_nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n \timg->line_nr = nr;\n }\n@@ -4287,19 +4284,19 @@ static void summary_patch_list(struct patch *patch)\n \n static void patch_stats(struct apply_state *state, struct patch *patch)\n {\n-\tint lines = patch->lines_added + patch->lines_deleted;\n+\tunsigned lines = patch->lines_added + patch->lines_deleted;\n \n \tif (lines > state->max_change)\n \t\tstate->max_change = lines;\n \tif (patch->old_name) {\n-\t\tint len = quote_c_style(patch->old_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->old_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->old_name);\n \t\tif (len > state->max_len)\n \t\t\tstate->max_len = len;\n \t}\n \tif (patch->new_name) {\n-\t\tint len = quote_c_style(patch->new_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->new_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->new_name);\n \t\tif (len > state->max_len)\ndiff --git a/apply.h b/apply.h\nindex 90e887ec0e..f53fbb1777 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -90,8 +90,8 @@ struct apply_state {\n \t * we've seen, and the longest filename. That allows us to do simple\n \t * scaling.\n \t */\n-\tint max_change;\n-\tint max_len;\n+\tunsigned max_change;\n+\tsize_t max_len;\n \n \t/*\n \t * Records filenames that have been touched, in order to handle\n@@ -170,7 +170,7 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t int *linenr,\n \t\t\t int p_value,\n \t\t\t const char *line,\n-\t\t\t int len,\n+\t\t\t size_t len,\n \t\t\t unsigned int size,\n \t\t\t struct patch *patch);\n \n\nbase-commit: bc204b742735ae06f65bb20291c95985c9633b7f\n-- \n2.43.0\n\n"},{"id":"511858","messageId":"Z6MXevArKhRLacAb@pks.im","threadId":"62902","inReplyTo":"20250205014055.737190-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-05T07:47:06Z","receivedAt":"2025-02-05T07:47:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 05, 2025 at 01:40:55AM +0000, Zejun Zhao wrote:\n> There are several -Wsign-comparison warnings in \"apply.c\", which can be\n> classified into the following three types:\n> \n> 1. comparing a `length` of `size_t` type with a result of pointer\n> arithmetic of `int` type\n> \n> 2. comparing a `length` of `size_t` type with a `length` of `int` type\n> \n> 3. comparing a loop count `i` of `int` type with an unsigned loop\n> bound\n> \n> Fix these warnings following one basic principle: do not touch the\n> relevant logics and keep the behaviors of the code. Adopt three\n> different strategies for each of the above three types:\n> \n> 1. cast the result of pointer arithmetic to `size_t` type\n> \n> 2. try to change the type of the `length` to `size_t` (may fall back\n> to Strategy 1 if the variable is not guaranteed to be unsigned)\n> \n> 3. use a loop count `i` of `size_t` type\n\nYou should split up this patch into a series, as it is really hard to\nfollow what's going on. There are a couple of things happening:\n\n - You change types in `struct apply_state`, which bubbles up.\n\n - You adapt `git_hdr_len()` to receive different inputs, which bubbles\n up.\n\n - You perform small fixes in several places.\n\nIt might also be a good idea to split out the loop counters into a\nseparate commit, as those are trivially correct.\n\n> Signed-off-by: Zejun Zhao \n> ---\n> apply.c | 73 +++++++++++++++++++++++++++------------------------------\n> apply.h | 6 ++---\n> 2 files changed, 38 insertions(+), 41 deletions(-)\n> \n> diff --git a/apply.c b/apply.c\n> index 4a7b6120ac..0c7b89dc3a 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -8,7 +8,6 @@\n> */\n> \n> #define USE_THE_REPOSITORY_VARIABLE\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n> \n> #include \"git-compat-util.h\"\n> #include \"abspath.h\"\n> @@ -540,7 +539,7 @@ static size_t date_len(const char *line, size_t len)\n> \t !isdigit(*p++) || !isdigit(*p++))\t/* Not a date. */\n> \t\treturn 0;\n> \n> -\tif (date - line >= strlen(\"19\") &&\n> +\tif ((size_t) (date - line) >= strlen(\"19\") &&\n\nWe know that `date` is always bigger than or equal to `line`, so this is\ncorrect.\n\n> @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,\n> \t * and optional space with octal mode.\n> \t */\n> \tconst char *ptr, *eol;\n> -\tint len;\n> -\tconst unsigned hexsz = the_hash_algo->hexsz;\n> +\tsize_t len;\n> +\tconst size_t hexsz = the_hash_algo->hexsz;\n\nThe change to `hexsz` shouldn't be needed, even if it makes us match the\ntype of `hexsz` as declared in `git_hash_algo`.\n\n> \tptr = strchr(line, '.');\n> -\tif (!ptr || ptr[1] != '.' || hexsz < ptr - line)\n> +\tif (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))\n\n`ptr` is the reline of `strrchr(line)`, so it must be either `NULL` or\ngreater than or equal to `line`, so this cast is fine.\n\n> @@ -1158,7 +1157,7 @@ static const char *skip_tree_prefix(int p_value,\n> */\n> static char *git_header_name(int p_value,\n> \t\t\t const char *line,\n> -\t\t\t int llen)\n> +\t\t\t size_t llen)\n> {\n> \tconst char *name;\n> \tconst char *second = NULL;\n\nIt would make sense to split this change out into a separate commit, as\nit bubbles up into calling functions, as well.\n\n> @@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value,\n> \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n> \t\tif (!cp)\n> \t\t\tgoto free_and_fail1;\n> -\t\tif (line + llen - cp != first.len ||\n> +\t\tif ((size_t) (line + llen - cp) != first.len ||\n> \t\t memcmp(first.buf, cp, first.len))\n> \t\t\tgoto free_and_fail1;\n> \t\treturn strbuf_detach(&first, NULL);\n\nThis cast should be fine, too.\n\n> @@ -1240,7 +1239,7 @@ static char *git_header_name(int p_value,\n> \t\t\t\tgoto free_and_fail2;\n> \n> \t\t\tlen = sp.buf + sp.len - np;\n> -\t\t\tif (len < second - name &&\n> +\t\t\tif (len < (size_t) (second - name) &&\n> \t\t\t !strncmp(np, name, len) &&\n> \t\t\t isspace(name[len])) {\n> \t\t\t\t/* Good */\n\nThis one, too. `second` is iterating through `name`, so it's always\ngreater or equal to `name`.\n\n> @@ -1371,14 +1370,13 @@ int parse_git_diff_header(struct strbuf *root,\n> \t\t\t{ \"index \", gitdiff_index },\n> \t\t\t{ \"\", gitdiff_unrecognized },\n> \t\t};\n> -\t\tint i;\n> \n> \t\tlen = linelen(line, size);\n> \t\tif (!len || line[len-1] != '\\n')\n> \t\t\tbreak;\n> -\t\tfor (i = 0; i < ARRAY_SIZE(optable); i++) {\n> +\t\tfor (size_t i = 0; i < ARRAY_SIZE(optable); i++) {\n> \t\t\tconst struct opentry *p = optable + i;\n> -\t\t\tint oplen = strlen(p->str);\n> +\t\t\tsize_t oplen = strlen(p->str);\n> \t\t\tint res;\n> \t\t\tif (len < oplen || memcmp(p->str, line, oplen))\n> \t\t\t\tcontinue;\n\nMakes sense.\n\n> @@ -1592,7 +1590,7 @@ static int find_header(struct apply_state *state,\n> \t\t\t\t\t\t\t\tsize, patch);\n> \t\t\tif (git_hdr_len < 0)\n> \t\t\t\treturn -128;\n> -\t\t\tif (git_hdr_len <= len)\n> +\t\t\tif ((size_t) git_hdr_len <= len)\n> \t\t\t\tcontinue;\n> \t\t\t*hdrsize = git_hdr_len;\n> \t\t\treturn offset;\n\nWe've asserted that `git_hdr_len` isn't negative, so the cast is fine.\n\n> @@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n> \t\t\t};\n> \t\t\tint i;\n\nThis may arguably be `size_t`, as well.\n\n> @@ -2257,12 +2255,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n> \t}\n> \n> \tif (patch->is_binary) {\n> -\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n> +\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n> \t\tstrbuf_release(&qname);\n> \t\treturn;\n> \t}\n> \n> -\tprintf(\" %-*s |\", max, qname.buf);\n> +\tprintf(\" %-*s |\", (int) max, qname.buf);\n> \tstrbuf_release(&qname);\n> \n> \t/*\n\nDo we _know_ that `max` fits into an `int`?\n\nPatrick\n"},{"id":"511868","messageId":"xmqqh658r1im.fsf@gitster.g","threadId":"62902","inReplyTo":"20250205014055.737190-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-05T12:58:57Z","receivedAt":"2025-02-05T12:59:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\n> @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,\n> \t * and optional space with octal mode.\n> \t */\n> \tconst char *ptr, *eol;\n> -\tint len;\n> -\tconst unsigned hexsz = the_hash_algo->hexsz;\n> +\tsize_t len;\n> +\tconst size_t hexsz = the_hash_algo->hexsz;\n\nI thought that I already saw this discussed in another thread.\n\nThe .hexsz of any hash algorithm would never be larger than what a\nplatform natural \"unsigned\" integer type can hold, so using size_t\nfor the member _is_ the wrong thing to do and the fix may be the\nother way around, no?\n\nThere are genuinely good changes (correcting assigned variable from\nint to size_t when the value ultimately came from a system function\nthat returns size_t) in this patch, but there are other annoying\nones like these, so I am not sure.\n\n> \tptr = strchr(line, '.');\n> -\tif (!ptr || ptr[1] != '.' || hexsz < ptr - line)\n> +\tif (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))\n\nIs this about -Wsign-compare complaining about size_t vs ptrdiff_t?\n\n> @@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value,\n> \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n> \t\tif (!cp)\n> \t\t\tgoto free_and_fail1;\n> -\t\tif (line + llen - cp != first.len ||\n> +\t\tif ((size_t) (line + llen - cp) != first.len ||\n\nDitto.\n\n> -\t\t\tif (len < second - name &&\n> +\t\t\tif (len < (size_t) (second - name) &&\n\nDitto.\n\nI said this before, but I am not sure if being strict about\n\"-Wsign-compare\" is really buying us much. If we are getting so\nmany false positives that need to be squelched by churning the code\nwith so many typecasts in order to find a new and real problem,\nis it really worth it?\n"},{"id":"511875","messageId":"20250205154232.237380-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"Z6MXevArKhRLacAb@pks.im","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-05T15:42:32Z","receivedAt":"2025-02-05T15:45:48Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Thank you for reviewing this patch.\n\nOn Wed, Feb 05, 2025 08:47:06 +0100, Patrick Steinhardt wrote:\n> You should split up this patch into a series, as it is really hard to\n> follow what's going on. There are a couple of things happening:\n> \n> - You change types in `struct apply_state`, which bubbles up.\n> \n> - You adapt `git_hdr_len()` to receive different inputs, which bubbles\n> up.\n> \n> - You perform small fixes in several places.\n> \n> It might also be a good idea to split out the loop counters into a\n> separate commit, as those are trivially correct.\n\nSure I'll come up with a v2 patch series, in which each kind of fixes \nwill be put in a single commit and I'll state why I believe the type \ncast/change is safe for every single fix in the commit message.\n\n> > @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,\n> > \t * and optional space with octal mode.\n> > \t */\n> > \tconst char *ptr, *eol;\n> > -\tint len;\n> > -\tconst unsigned hexsz = the_hash_algo->hexsz;\n> > +\tsize_t len;\n> > +\tconst size_t hexsz = the_hash_algo->hexsz;\n> \n> The change to `hexsz` shouldn't be needed, even if it makes us match the\n> type of `hexsz` as declared in `git_hash_algo`.\n\nYes it's not necessary here to change the type. And for the `hexsz` stuff,\non Wed, Feb 05 2025 04:58:57 -0800, Junio C Hamano wrote,\n> I thought that I already saw this discussed in another thread.\n> \n> The .hexsz of any hash algorithm would never be larger than what a\n> platform natural \"unsigned\" integer type can hold, so using size_t\n> for the member _is_ the wrong thing to do and the fix may be the\n> other way around, no?\n\nI found the discussion mentioned at [1]. It seems like the change here \nonly makes things worse so I'll see if I'd leave it untouched or change \nthe type of `.hexsz` member to `int` or something.\n\n[1] https://lore.kernel.org/git/xmqqttaqw2eb.fsf@gitster.g/\n\n> > @@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n> > \t\t\t};\n> > \t\t\tint i;\n> \n> This may arguably be `size_t`, as well.\n\nIt's OK for me to use a `size_t` loop count everywhere but I tried to \nkeep the changes in this patch minimal (forget about the `hexsz` thing).\nI could apply this change if you insist.\n\n> > @@ -2257,12 +2255,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n> > \t}\n> > \n> > \tif (patch->is_binary) {\n> > -\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n> > +\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n> > \t\tstrbuf_release(&qname);\n> > \t\treturn;\n> > \t}\n> > \n> > -\tprintf(\" %-*s |\", max, qname.buf);\n> > +\tprintf(\" %-*s |\", (int) max, qname.buf);\n> > \tstrbuf_release(&qname);\n> > \n> > \t/*\n> \n> Do we _know_ that `max` fits into an `int`?\n\nYep we've set an upper bound for `max` before:\n> /*\n> * \"scale\" the filename\n> */\n> max = state->max_len;\n> if (max > 50)\n> \t max = 50;\nso it must fit into an `int`.\n"},{"id":"511887","messageId":"20250205164947.281934-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"xmqqh658r1im.fsf@gitster.g","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-05T16:49:47Z","receivedAt":"2025-02-05T16:50:33Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Thank you for reviewing this patch.\n\nOn Wed, Feb 05 2025 04:58:57 -0800, Junio C Hamano wrote,\n> > @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state,\n> > \t * and optional space with octal mode.\n> > \t */\n> > \tconst char *ptr, *eol;\n> > -\tint len;\n> > -\tconst unsigned hexsz = the_hash_algo->hexsz;\n> > +\tsize_t len;\n> > +\tconst size_t hexsz = the_hash_algo->hexsz;\n> \n> I thought that I already saw this discussed in another thread.\n> \n> The .hexsz of any hash algorithm would never be larger than what a\n> platform natural \"unsigned\" integer type can hold, so using size_t\n> for the member _is_ the wrong thing to do and the fix may be the\n> other way around, no?\n\nSorry I didn't saw the comment at [1]. You are right about this. I \nprefer changing types of the `git_hash_algo::*sz` family to `unsigned`.\n\n[1] https://lore.kernel.org/git/xmqqttaqw2eb.fsf@gitster.g/\n\n\n> > \tptr = strchr(line, '.');\n> > -\tif (!ptr || ptr[1] != '.' || hexsz < ptr - line)\n> > +\tif (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))\n> \n> Is this about -Wsign-compare complaining about size_t vs ptrdiff_t?\n> \n> > @@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value,\n> > \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n> > \t\tif (!cp)\n> > \t\t\tgoto free_and_fail1;\n> > -\t\tif (line + llen - cp != first.len ||\n> > +\t\tif ((size_t) (line + llen - cp) != first.len ||\n> \n> Ditto.\n> \n> > -\t\t\tif (len < second - name &&\n> > +\t\t\tif (len < (size_t) (second - name) &&\n> \n> Ditto.\n\nYes it's comparing unsigned (`size_t`) with signed (`ptrdiff_t`).\n\n> I said this before, but I am not sure if being strict about\n> \"-Wsign-compare\" is really buying us much. If we are getting so\n> many false positives that need to be squelched by churning the code\n> with so many typecasts in order to find a new and real problem,\n> is it really worth it?\n\nWell, I contributed most before to a Rust OS kernel project so \nhonestly I'm used to the idea of explicit type casting when necessary \nto avoid possible bugs. However, this is non-trivial. People should \nunderstand each relevant variable's semantics to decide what its type \nshould be and do typecast on top of that. Even if it's already made a \ngradual process, it may still cost much from both the contributors and \nthe reviewers but brings benefits that do not match up. Maybe a clear \ncommit message that states why each type cast/change makes sense can \nhelp with the iteration? I'm not quite sure about that.\n"},{"id":"512149","messageId":"20250209081216.241350-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250205014055.737190-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 0/6] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:10Z","receivedAt":"2025-02-09T08:13:06Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"There are several -Wsign-comparison warnings in \"apply.c\", which can be\nclassified into the following three types:\n\n 1. comparing a length of `size_t` type with a `ptrdiff_t` value\n\n 2. comparing a length of `size_t` type with a length of `int` type\n\n 3. comparing a loop counter `i` of `int` type with an unsigned loop\n bound\n\nFix these warnings following one basic principle: do not touch the\nrelevant logics and keep the behaviors of the code. Adopt three\ndifferent strategies for each of the above three types:\n\n 1. cast the `ptrdiff_t` values to `size_t` type\n\n 2. try to change the type of the `int` length to `size_t` (may fall \n back to Strategy 1 if the variable is not guaranteed to be unsigned)\n\n 3. use a loop counter `i` of `size_t` type\n\nZejun Zhao (6):\n apply: change fields in `apply_state` to unsigned\n apply: change some variables from `int` to `size_t`\n apply: do a typecast to eliminate warnings\n apply: cast some ptrdiff_t's to size_t's\n apply: use `size_t` loop counters\n apply: enable -Wsign-comparison checks\n\n apply.c | 69 +++++++++++++++++++++++++++------------------------------\n apply.h | 4 ++--\n 2 files changed, 35 insertions(+), 38 deletions(-)\n\n\nbase-commit: bc204b742735ae06f65bb20291c95985c9633b7f\n-- \n2.43.0\n\n"},{"id":"512150","messageId":"20250209081216.241350-2-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:11Z","receivedAt":"2025-02-09T08:13:30Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"`.max_change` and `.max_len` of `apply_state` are only used as unsigned\nintegers. Misuse of `int` type would cause -Wsign-comparison warnings.\n\nFix this by\n\n - change `.max_change`'s type to `unsigned` since it's just a counter\n\n - change `.max_len`'s type to `size_t` since it's a length\n\n - change the types of relevant variables in function `show_stats`\n\nNote that `printf`'s format string requires us to do some typecast\n(from `size_t` to `int`) on `max` in function `show_stats`. This is\nsafe because we already set a upper bound of `50` for `max` before the\ncast.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 9 +++++----\n apply.h | 4 ++--\n 2 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4a7b6120ac..831b338155 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n {\n \tstruct strbuf qname = STRBUF_INIT;\n \tchar *cp = patch->new_name ? patch->new_name : patch->old_name;\n-\tint max, add, del;\n+\tsize_t max;\n+\tunsigned add, del;\n \n \tquote_c_style(cp, &qname, NULL, 0);\n \n@@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n \t}\n \n \tif (patch->is_binary) {\n-\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n+\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n \t\tstrbuf_release(&qname);\n \t\treturn;\n \t}\n \n-\tprintf(\" %-*s |\", max, qname.buf);\n+\tprintf(\" %-*s |\", (int) max, qname.buf);\n \tstrbuf_release(&qname);\n \n \t/*\n@@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n \tdel = patch->lines_deleted;\n \n \tif (state->max_change > 0) {\n-\t\tint total = ((add + del) * max + state->max_change / 2) / state->max_change;\n+\t\tunsigned total = ((add + del) * max + state->max_change / 2) / state->max_change;\n \t\tadd = (add * max + state->max_change / 2) / state->max_change;\n \t\tdel = total - add;\n \t}\ndiff --git a/apply.h b/apply.h\nindex 90e887ec0e..f7f369d44f 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -90,8 +90,8 @@ struct apply_state {\n \t * we've seen, and the longest filename. That allows us to do simple\n \t * scaling.\n \t */\n-\tint max_change;\n-\tint max_len;\n+\tunsigned max_change;\n+\tsize_t max_len;\n \n \t/*\n \t * Records filenames that have been touched, in order to handle\n-- \n2.43.0\n\n"},{"id":"512151","messageId":"20250209081216.241350-3-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 2/6] apply: change some variables from `int` to `size_t`","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:12Z","receivedAt":"2025-02-09T08:13:36Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Some assigned variables are mistyped as `int`, including\n\n - those whose values come from a system function returning `size_t`,\n\n - those that are used for array indexing,\n\n - those that represent length/size/distance,\n\nsome of which will trigger -Wsign-comparison warnings.\n\nChange some of them to `size_t`/`unsigned`.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 831b338155..b4ae74a5fb 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,\n \t * and optional space with octal mode.\n \t */\n \tconst char *ptr, *eol;\n-\tint len;\n+\tsize_t len;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n \tptr = strchr(line, '.');\n@@ -2185,7 +2185,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t};\n \t\t\tint i;\n \t\t\tfor (i = 0; binhdr[i]; i++) {\n-\t\t\t\tint len = strlen(binhdr[i]);\n+\t\t\t\tsize_t len = strlen(binhdr[i]);\n \t\t\t\tif (len < size - hd &&\n \t\t\t\t !memcmp(binhdr[i], buffer + hd, len)) {\n \t\t\t\t\tstate->linenr++;\n@@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tstruct image fixed_preimage = IMAGE_INIT;\n \tsize_t insert_pos = 0;\n-\tint i, ctx, reduced;\n+\tint i, reduced;\n+\tsize_t ctx;\n \tconst char *fixed;\n \n \t/*\n@@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state,\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n \tsize_t fixed_len;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \tint ret;\n \n \tif (preimage->line_nr + current_lno <= img->line_nr) {\n@@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,\n {\n \tint i;\n \tunsigned long backwards, forwards, current;\n-\tint backwards_lno, forwards_lno, current_lno;\n+\tsize_t backwards_lno, forwards_lno, current_lno;\n \n \t/*\n \t * When running with --allow-overlap, it is possible that a hunk is\n@@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state,\n */\n static void update_image(struct apply_state *state,\n \t\t\t struct image *img,\n-\t\t\t int applied_pos,\n+\t\t\t size_t applied_pos,\n \t\t\t struct image *preimage,\n \t\t\t struct image *postimage)\n {\n@@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state,\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tsize_t result_alloc;\n \tchar *result;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \n \t/*\n \t * If we are removing blank lines at the end of img,\n@@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch)\n \n static void patch_stats(struct apply_state *state, struct patch *patch)\n {\n-\tint lines = patch->lines_added + patch->lines_deleted;\n+\tunsigned lines = patch->lines_added + patch->lines_deleted;\n \n \tif (lines > state->max_change)\n \t\tstate->max_change = lines;\n \tif (patch->old_name) {\n-\t\tint len = quote_c_style(patch->old_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->old_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->old_name);\n \t\tif (len > state->max_len)\n \t\t\tstate->max_len = len;\n \t}\n \tif (patch->new_name) {\n-\t\tint len = quote_c_style(patch->new_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->new_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->new_name);\n \t\tif (len > state->max_len)\n-- \n2.43.0\n\n"},{"id":"512152","messageId":"20250209081216.241350-4-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 3/6] apply: do a typecast to eliminate warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:13Z","receivedAt":"2025-02-09T08:13:41Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"`git_hdr_len` is an `int` variable that can be negative and is used to\ncompare against a `len` of `size_t`, which will trigger\n-Wsign-comparison warnings\n\nCast `git_hdr_len` to `size_t` after an above-zero check.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex b4ae74a5fb..605a0aa2e3 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1592,7 +1592,7 @@ static int find_header(struct apply_state *state,\n \t\t\t\t\t\t\t\tsize, patch);\n \t\t\tif (git_hdr_len < 0)\n \t\t\t\treturn -128;\n-\t\t\tif (git_hdr_len <= len)\n+\t\t\tif ((size_t) git_hdr_len <= len)\n \t\t\t\tcontinue;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\n-- \n2.43.0\n\n"},{"id":"512153","messageId":"20250209081216.241350-5-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 4/6] apply: cast some ptrdiff_t's to size_t's","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:14Z","receivedAt":"2025-02-09T08:13:44Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"There are several -Wsign-comparison warnings in \"apply.c\", complaining\nabout us comparing ptrdiff_t's with size_t's.\n\nFix these warnings by typecasting from ptrdiff_t to size_t. As to why\nthe casts is safe,\n\n - in function `date_len`, `date` is the starting address of a date at\n the end of the `line` and is guaranteed to be larger than (or equal\n to) `line`\n\n - in function `git_header_name`, `cp` is guaranteed to be larger than\n (or equal to) `second`, so `line + len` is greater than (or equal to)\n `cp` since we already treat `line + len - second` as a size_t\n\n - in function `git_header_name`, we are iterating `name` using\n `second`, so `second` is guaranteed to be greater than (or equal to)\n `name`\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 605a0aa2e3..72464fb6c2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -540,7 +540,7 @@ static size_t date_len(const char *line, size_t len)\n \t !isdigit(*p++) || !isdigit(*p++))\t/* Not a date. */\n \t\treturn 0;\n \n-\tif (date - line >= strlen(\"19\") &&\n+\tif ((size_t) (date - line) >= strlen(\"19\") &&\n \t isdigit(date[-1]) && isdigit(date[-2]))\t/* 4-digit year */\n \t\tdate -= strlen(\"19\");\n \n@@ -1207,7 +1207,7 @@ static char *git_header_name(int p_value,\n \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n \t\tif (!cp)\n \t\t\tgoto free_and_fail1;\n-\t\tif (line + llen - cp != first.len ||\n+\t\tif ((size_t) (line + llen - cp) != first.len ||\n \t\t memcmp(first.buf, cp, first.len))\n \t\t\tgoto free_and_fail1;\n \t\treturn strbuf_detach(&first, NULL);\n@@ -1240,7 +1240,7 @@ static char *git_header_name(int p_value,\n \t\t\t\tgoto free_and_fail2;\n \n \t\t\tlen = sp.buf + sp.len - np;\n-\t\t\tif (len < second - name &&\n+\t\t\tif (len < (size_t) (second - name) &&\n \t\t\t !strncmp(np, name, len) &&\n \t\t\t isspace(name[len])) {\n \t\t\t\t/* Good */\n-- \n2.43.0\n\n"},{"id":"512154","messageId":"20250209081216.241350-6-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 5/6] apply: use `size_t` loop counters","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:15Z","receivedAt":"2025-02-09T08:13:48Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Some `int` loop counters trigger -Wsign-comparison warnings.\n\nUse `size_t` loop counters.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 32 ++++++++++++++------------------\n 1 file changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 72464fb6c2..585f534732 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1371,12 +1371,11 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t{ \"index \", gitdiff_index },\n \t\t\t{ \"\", gitdiff_unrecognized },\n \t\t};\n-\t\tint i;\n \n \t\tlen = linelen(line, size);\n \t\tif (!len || line[len-1] != '\\n')\n \t\t\tbreak;\n-\t\tfor (i = 0; i < ARRAY_SIZE(optable); i++) {\n+\t\tfor (size_t i = 0; i < ARRAY_SIZE(optable); i++) {\n \t\t\tconst struct opentry *p = optable + i;\n \t\t\tint oplen = strlen(p->str);\n \t\t\tint res;\n@@ -2097,7 +2096,6 @@ static void add_name_limit(struct apply_state *state,\n static int use_patch(struct apply_state *state, struct patch *p)\n {\n \tconst char *pathname = p->new_name ? p->new_name : p->old_name;\n-\tint i;\n \n \t/* Paths outside are not touched regardless of \"--include\" */\n \tif (state->prefix && *state->prefix) {\n@@ -2107,7 +2105,7 @@ static int use_patch(struct apply_state *state, struct patch *p)\n \t}\n \n \t/* See if it matches any of exclude/include rule */\n-\tfor (i = 0; i < state->limit_by_name.nr; i++) {\n+\tfor (size_t i = 0; i < state->limit_by_name.nr; i++) {\n \t\tstruct string_list_item *it = &state->limit_by_name.items[i];\n \t\tif (!wildmatch(it->string, pathname, 0))\n \t\t\treturn (it->util != NULL);\n@@ -2183,8 +2181,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t\t\"Files \",\n \t\t\t\tNULL,\n \t\t\t};\n-\t\t\tint i;\n-\t\t\tfor (i = 0; binhdr[i]; i++) {\n+\t\t\tfor (size_t i = 0; binhdr[i]; i++) {\n \t\t\t\tsize_t len = strlen(binhdr[i]);\n \t\t\t\tif (len < size - hd &&\n \t\t\t\t !memcmp(binhdr[i], buffer + hd, len)) {\n@@ -2320,7 +2317,7 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tstruct image fixed_preimage = IMAGE_INIT;\n \tsize_t insert_pos = 0;\n-\tint i, reduced;\n+\tint reduced;\n \tsize_t ctx;\n \tconst char *fixed;\n \n@@ -2330,7 +2327,7 @@ static void update_pre_post_images(struct image *preimage,\n \t * free \"oldlines\".\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n-\tfor (i = 0; i < fixed_preimage.line_nr; i++)\n+\tfor (size_t i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n \timage_clear(preimage);\n \t*preimage = fixed_preimage;\n@@ -2339,7 +2336,7 @@ static void update_pre_post_images(struct image *preimage,\n \t/*\n \t * Adjust the common context lines in postimage.\n \t */\n-\tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n+\tfor (size_t i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n \n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n@@ -2419,9 +2416,9 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t struct image *postimage,\n \t\t\t\t unsigned long current,\n \t\t\t\t int current_lno,\n-\t\t\t\t int preimage_limit)\n+\t\t\t\t size_t preimage_limit)\n {\n-\tint i;\n+\tsize_t i;\n \tsize_t imgoff = 0;\n \tsize_t preoff = 0;\n \tsize_t extra_chars;\n@@ -2488,7 +2485,7 @@ static int match_fragment(struct apply_state *state,\n \t\t\t unsigned ws_rule,\n \t\t\t int match_beginning, int match_end)\n {\n-\tint i;\n+\tsize_t i;\n \tconst char *orig, *target;\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n@@ -2665,12 +2662,11 @@ static int match_fragment(struct apply_state *state,\n \tfor ( ; i < preimage->line_nr; i++) {\n \t\tsize_t fixstart = fixed.len; /* start of the fixed preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n-\t\tint j;\n \n \t\t/* Try fixing the line in the preimage */\n \t\tws_fix_copy(&fixed, orig, oldlen, ws_rule, NULL);\n \n-\t\tfor (j = fixstart; j < fixed.len; j++) {\n+\t\tfor (size_t j = fixstart; j < fixed.len; j++) {\n \t\t\tif (!isspace(fixed.buf[j])) {\n \t\t\t\tret = 0;\n \t\t\t\tgoto out;\n@@ -2800,7 +2796,7 @@ static void update_image(struct apply_state *state,\n \t * remove the copy of preimage at offset in img\n \t * and replace it with postimage\n \t */\n-\tint i, nr;\n+\tint nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tsize_t result_alloc;\n \tchar *result;\n@@ -2819,11 +2815,11 @@ static void update_image(struct apply_state *state,\n \tif (preimage_limit > img->line_nr - applied_pos)\n \t\tpreimage_limit = img->line_nr - applied_pos;\n \n-\tfor (i = 0; i < applied_pos; i++)\n+\tfor (size_t i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n \n \tremove_count = 0;\n-\tfor (i = 0; i < preimage_limit; i++)\n+\tfor (size_t i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n \tinsert_count = postimage->buf.len;\n \n@@ -2852,7 +2848,7 @@ static void update_image(struct apply_state *state,\n \t\t\t img->line_nr - (applied_pos + preimage_limit));\n \tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);\n \tif (!state->allow_overlap)\n-\t\tfor (i = 0; i < postimage->line_nr; i++)\n+\t\tfor (size_t i = 0; i < postimage->line_nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n \timg->line_nr = nr;\n }\n-- \n2.43.0\n\n"},{"id":"512155","messageId":"20250209081216.241350-7-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v2 6/6] apply: enable -Wsign-comparison checks","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-09T08:12:16Z","receivedAt":"2025-02-09T08:13:51Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Remove the `#define DISABLE_SIGN_COMPARE_WARNINGS` header line.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 585f534732..51d0f4813f 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -8,7 +8,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n-- \n2.43.0\n\n"},{"id":"512345","messageId":"Z62MTZXQD3Wa47Jz@pks.im","threadId":"62902","inReplyTo":"20250209081216.241350-3-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH v2 2/6] apply: change some variables from `int` to `size_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-13T06:08:13Z","receivedAt":"2025-02-13T06:08:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 09, 2025 at 08:12:12AM +0000, Zejun Zhao wrote:\n> Some assigned variables are mistyped as `int`, including\n> \n> - those whose values come from a system function returning `size_t`,\n> \n> - those that are used for array indexing,\n> \n> - those that represent length/size/distance,\n> \n> some of which will trigger -Wsign-comparison warnings.\n> \n> Change some of them to `size_t`/`unsigned`.\n> \n> Signed-off-by: Zejun Zhao \n> ---\n> apply.c | 21 +++++++++++----------\n> 1 file changed, 11 insertions(+), 10 deletions(-)\n> \n> diff --git a/apply.c b/apply.c\n> index 831b338155..b4ae74a5fb 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,\n> \t * and optional space with octal mode.\n> \t */\n> \tconst char *ptr, *eol;\n> -\tint len;\n> +\tsize_t len;\n> \tconst unsigned hexsz = the_hash_algo->hexsz;\n> \n> \tptr = strchr(line, '.');\n\nThis is storing the result of `ptr - line`, which will be a positive\ninteger. It's later passed to `memcpy()`, which expects a `size_t`.\n\n> @@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage,\n> {\n> \tstruct image fixed_preimage = IMAGE_INIT;\n> \tsize_t insert_pos = 0;\n> -\tint i, ctx, reduced;\n> +\tint i, reduced;\n> +\tsize_t ctx;\n> \tconst char *fixed;\n> \n> \t/*\n\n`ctx` is indexing an array and counts against `preimage->line_nr`, which\nis a `size_t`, too.\n\n> @@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state,\n> \tstruct strbuf fixed = STRBUF_INIT;\n> \tchar *fixed_buf;\n> \tsize_t fixed_len;\n> -\tint preimage_limit;\n> +\tsize_t preimage_limit;\n> \tint ret;\n> \n> \tif (preimage->line_nr + current_lno <= img->line_nr) {\n\nThis one stores `struct image::line_nr`, which is a `size_t`.\n\n> @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,\n> {\n> \tint i;\n> \tunsigned long backwards, forwards, current;\n> -\tint backwards_lno, forwards_lno, current_lno;\n> +\tsize_t backwards_lno, forwards_lno, current_lno;\n> \n> \t/*\n> \t * When running with --allow-overlap, it is possible that a hunk is\n\nThese are a bit curious, as they store `line`, which is itself an `int`\nparameter. As far as I understand, the only caller is also only ever\npassing a positive integer here.\n\n> @@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state,\n> */\n> static void update_image(struct apply_state *state,\n> \t\t\t struct image *img,\n> -\t\t\t int applied_pos,\n> +\t\t\t size_t applied_pos,\n> \t\t\t struct image *preimage,\n> \t\t\t struct image *postimage)\n> {\n> @@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state,\n> \tsize_t remove_count, insert_count, applied_at = 0;\n> \tsize_t result_alloc;\n> \tchar *result;\n> -\tint preimage_limit;\n> +\tsize_t preimage_limit;\n> \n> \t/*\n> \t * If we are removing blank lines at the end of img,\n\nThe caller makes sure that the function only gets called when the\nparameter is a positive integer.\n\n> @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch)\n> \n> static void patch_stats(struct apply_state *state, struct patch *patch)\n> {\n> -\tint lines = patch->lines_added + patch->lines_deleted;\n> +\tunsigned lines = patch->lines_added + patch->lines_deleted;\n\nThis one is curious again, as the type of these variables is an `int`.\nThis should likely be adapted in tandem if they cannot be negative.\n\nPatrick\n"},{"id":"512346","messageId":"Z62MVV_GfzTWIn7W@pks.im","threadId":"62902","inReplyTo":"20250209081216.241350-6-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH v2 5/6] apply: use `size_t` loop counters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-13T06:08:21Z","receivedAt":"2025-02-13T06:08:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 09, 2025 at 08:12:15AM +0000, Zejun Zhao wrote:\n> diff --git a/apply.c b/apply.c\n> index 72464fb6c2..585f534732 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2419,9 +2416,9 @@ static int line_by_line_fuzzy_match(struct image *img,\n> \t\t\t\t struct image *postimage,\n> \t\t\t\t unsigned long current,\n> \t\t\t\t int current_lno,\n> -\t\t\t\t int preimage_limit)\n> +\t\t\t\t size_t preimage_limit)\n> {\n> -\tint i;\n> +\tsize_t i;\n> \tsize_t imgoff = 0;\n> \tsize_t preoff = 0;\n> \tsize_t extra_chars;\n\nThis one does more than just changing the loop counters. Probably makes\nsense to split out into a separate commit.\n\nPatrick\n"},{"id":"512358","messageId":"CAOLa=ZQbVBmGnsvWUz0ByW8z5NMgLY2oZZ1RLJeoWJST9PGAvQ@mail.gmail.com","threadId":"62902","inReplyTo":"20250209081216.241350-2-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-13T09:51:12Z","receivedAt":"2025-02-13T09:51:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Zejun Zhao writes:\n\n> `.max_change` and `.max_len` of `apply_state` are only used as unsigned\n> integers. Misuse of `int` type would cause -Wsign-comparison warnings.\n>\n> Fix this by\n>\n> - change `.max_change`'s type to `unsigned` since it's just a counter\n>\n\nLooking at `.max_change` it seems like this is only assigned in\n`patch_stats()` where we do\n\n int lines = patch->lines_added + patch->lines_deleted;\n\n if (lines > state->max_change)\n state->max_change = lines;\n\nIn this case shouldn't we first convert `.lines_added` `.lines_deleted`\nto also be 'unsigned int' in the first place?\n\n> - change `.max_len`'s type to `size_t` since it's a length\n>\n\nI see that this is assigned the return value of `strlen()` so this\nmakes sense.\n\n> - change the types of relevant variables in function `show_stats`\n>\n> Note that `printf`'s format string requires us to do some typecast\n> (from `size_t` to `int`) on `max` in function `show_stats`. This is\n> safe because we already set a upper bound of `50` for `max` before the\n> cast.\n>\n> Signed-off-by: Zejun Zhao \n> ---\n> apply.c | 9 +++++----\n> apply.h | 4 ++--\n> 2 files changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 4a7b6120ac..831b338155 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n> {\n> \tstruct strbuf qname = STRBUF_INIT;\n> \tchar *cp = patch->new_name ? patch->new_name : patch->old_name;\n> -\tint max, add, del;\n> +\tsize_t max;\n> +\tunsigned add, del;\n>\n\nTangential to this patch, I don't think we have a guideline on using\n'unsigned' vs 'unsigned int'. Probably we should.\n\n> \tquote_c_style(cp, &qname, NULL, 0);\n>\n> @@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n> \t}\n>\n> \tif (patch->is_binary) {\n> -\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n> +\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n> \t\tstrbuf_release(&qname);\n> \t\treturn;\n> \t}\n>\n> -\tprintf(\" %-*s |\", max, qname.buf);\n> +\tprintf(\" %-*s |\", (int) max, qname.buf);\n> \tstrbuf_release(&qname);\n>\n> \t/*\n> @@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n> \tdel = patch->lines_deleted;\n>\n> \tif (state->max_change > 0) {\n> -\t\tint total = ((add + del) * max + state->max_change / 2) / state->max_change;\n> +\t\tunsigned total = ((add + del) * max + state->max_change / 2) / state->max_change;\n> \t\tadd = (add * max + state->max_change / 2) / state->max_change;\n> \t\tdel = total - add;\n> \t}\n> diff --git a/apply.h b/apply.h\n> index 90e887ec0e..f7f369d44f 100644\n> --- a/apply.h\n> +++ b/apply.h\n> @@ -90,8 +90,8 @@ struct apply_state {\n> \t * we've seen, and the longest filename. That allows us to do simple\n> \t * scaling.\n> \t */\n> -\tint max_change;\n> -\tint max_len;\n> +\tunsigned max_change;\n> +\tsize_t max_len;\n>\n> \t/*\n> \t * Records filenames that have been touched, in order to handle\n> --\n> 2.43.0\n\nThe rest look good.\n"},{"id":"512376","messageId":"xmqqikpdofjd.fsf@gitster.g","threadId":"62902","inReplyTo":"CAOLa=ZQbVBmGnsvWUz0ByW8z5NMgLY2oZZ1RLJeoWJST9PGAvQ@mail.gmail.com","subject":"Re: [GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-13T18:39:18Z","receivedAt":"2025-02-13T18:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Zejun Zhao writes:\n>\n>> `.max_change` and `.max_len` of `apply_state` are only used as unsigned\n>> integers. Misuse of `int` type would cause -Wsign-comparison warnings.\n>>\n>> Fix this by\n>>\n>> - change `.max_change`'s type to `unsigned` since it's just a counter\n>>\n>\n> Looking at `.max_change` it seems like this is only assigned in\n> `patch_stats()` where we do\n>\n> int lines = patch->lines_added + patch->lines_deleted;\n>\n> if (lines > state->max_change)\n> state->max_change = lines;\n>\n> In this case shouldn't we first convert `.lines_added` `.lines_deleted`\n> to also be 'unsigned int' in the first place?\n\nSurely. Or if any of the internal API uses a calling convention\nthat yields number of lines on success and negative number to signal\nerrors, we could also unify them to signed integer instead. In\neither case, using types consistently is good, and thanks for sharp\neyes spotting this instance.\n\n>> @@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n>> \t}\n>>\n>> \tif (patch->is_binary) {\n>> -\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n>> +\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n>> \t\tstrbuf_release(&qname);\n>> \t\treturn;\n>> \t}\n>>\n>> -\tprintf(\" %-*s |\", max, qname.buf);\n>> +\tprintf(\" %-*s |\", (int) max, qname.buf);\n>> \tstrbuf_release(&qname);\n>>\n\nThis is the kind of fallout that makes the resulting code harder to\nread. How bad would the code churn be if we instead unify to the\nsigned integer type, instead of using size_t, and making sure we\nuse the range-checking versions of arithmetic when needed, I have to\nwonder?\n"},{"id":"512474","messageId":"20250216071202.61372-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"Z62MTZXQD3Wa47Jz@pks.im","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:12:02Z","receivedAt":"2025-02-16T07:12:28Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote,\n> > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,\n> > {\n> > \tint i;\n> > \tunsigned long backwards, forwards, current;\n> > -\tint backwards_lno, forwards_lno, current_lno;\n> > +\tsize_t backwards_lno, forwards_lno, current_lno;\n> > \n> > \t/*\n> > \t * When running with --allow-overlap, it is possible that a hunk is\n> \n> These are a bit curious, as they store `line`, which is itself an `int`\n> parameter. As far as I understand, the only caller is also only ever\n> passing a positive integer here.\n\nThey do store an `int` parameter, which is `line`. However, we cannot change \n`line` to unsigned since it can temporarily store an negative value before \nthe assignments to these `*_lno` happen. Below are from function `find_pos` \n(without any change).\n\n /*\n * If match_beginning or match_end is specified, there is no\n * point starting from a wrong line that will never match and\n * wander around and wait for a match at the specified end.\n */\n if (match_beginning)\n\t line = 0;\n else if (match_end)\n\t line = img->line_nr - preimage->line_nr;\n\n /*\n * Because the comparison is unsigned, the following test\n * will also take care of a negative line number that can\n * result when match_end and preimage is larger than the target.\n */\n if (line > img->line_nr)\n\t line = img->line_nr;\n\n> > @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch)\n> > \n> > static void patch_stats(struct apply_state *state, struct patch *patch)\n> > {\n> > -\tint lines = patch->lines_added + patch->lines_deleted;\n> > +\tunsigned lines = patch->lines_added + patch->lines_deleted;\n> \n> This one is curious again, as the type of these variables is an `int`.\n> This should likely be adapted in tandem if they cannot be negative.\n\nThank you for identifying. Will be in next version.\n"},{"id":"512475","messageId":"20250216072843.72385-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250205014055.737190-1-jelly.zhao.42@gmail.com","subject":"[GSOC][PATCH v3 0/6] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:37Z","receivedAt":"2025-02-16T07:29:09Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"This is the v3 of this patch set.\n\nChanges from v2:\n\n - change `patch::lines_added/deleted` to unsigned\n\n - use `unsigned int` instead of `unsigned`\n\n - separate irrelevant changes from the \"loop counter\" commit\n\nBelow are the cover letter from the v2.\n\nThere are several -Wsign-comparison warnings in \"apply.c\", which can be\nclassified into the following three types:\n\n 1. comparing a length of `size_t` type with a `ptrdiff_t` value\n\n 2. comparing a length of `size_t` type with a length of `int` type\n\n 3. comparing a loop counter `i` of `int` type with an unsigned loop\n bound\n\nFix these warnings following one basic principle: do not touch the\nrelevant logics and keep the behaviors of the code. Adopt three\ndifferent strategies for each of the above three types:\n\n 1. cast the `ptrdiff_t` values to `size_t` type\n\n 2. try to change the type of the `int` length to `size_t` (may fall \n back to Strategy 1 if the variable is not guaranteed to be unsigned)\n\n 3. use a loop counter `i` of `size_t` type\n\nZejun Zhao (6):\n apply: correct type misuse in `apply.h`\n apply: change some variables from `int` to `size_t`\n apply: do a typecast to eliminate warnings\n apply: cast some ptrdiff_t's to size_t's\n apply: use `size_t` loop counters\n apply: enable -Wsign-comparison checks\n\n apply.c | 69 +++++++++++++++++++++++++++------------------------------\n apply.h | 6 ++---\n 2 files changed, 36 insertions(+), 39 deletions(-)\n\nRange-diff against v2:\n1: 16ee2f93a3 ! 1: a5985e0e16 apply: change fields in `apply_state` to unsigned\n @@ Metadata\n Author: Zejun Zhao \n \n ## Commit message ##\n - apply: change fields in `apply_state` to unsigned\n + apply: correct type misuse in `apply.h`\n \n - `.max_change` and `.max_len` of `apply_state` are only used as unsigned\n - integers. Misuse of `int` type would cause -Wsign-comparison warnings.\n + There are a few `int` fields of structs in `apply.h` that are only used\n + as unsigned integer and are only assigned unsigned integer value. Some\n + of these misuse of `int` type would cause -Wsign-comparison warnings.\n \n Fix this by\n \n - - change `.max_change`'s type to `unsigned` since it's just a counter\n + - change `apply_state::max_change`'s type to `unsigned int` since it's\n + just a counter of lines\n \n - - change `.max_len`'s type to `size_t` since it's a length\n + - change `apply_state::max_len`'s type to `size_t` since it's storing\n + a length\n \n - - change the types of relevant variables in function `show_stats`\n + - change `patch::lines_added/deleted`'s types to `unsigned int` since\n + they are just counters of lines\n +\n + - change the types of relevant variables in function `show_stats` and\n + `patch_stats`\n \n Note that `printf`'s format string requires us to do some typecast\n (from `size_t` to `int`) on `max` in function `show_stats`. This is\n @@ apply.c: static void show_stats(struct apply_state *state, struct patch *patch)\n \tchar *cp = patch->new_name ? patch->new_name : patch->old_name;\n -\tint max, add, del;\n +\tsize_t max;\n -+\tunsigned add, del;\n ++\tunsigned int add, del;\n \n \tquote_c_style(cp, &qname, NULL, 0);\n \n @@ apply.c: static void show_stats(struct apply_state *state, struct patch *patch)\n \n \tif (state->max_change > 0) {\n -\t\tint total = ((add + del) * max + state->max_change / 2) / state->max_change;\n -+\t\tunsigned total = ((add + del) * max + state->max_change / 2) / state->max_change;\n ++\t\tunsigned int total = ((add + del) * max + state->max_change / 2) / state->max_change;\n \t\tadd = (add * max + state->max_change / 2) / state->max_change;\n \t\tdel = total - add;\n \t}\n +@@ apply.c: static void summary_patch_list(struct patch *patch)\n + \n + static void patch_stats(struct apply_state *state, struct patch *patch)\n + {\n +-\tint lines = patch->lines_added + patch->lines_deleted;\n ++\tunsigned int lines = patch->lines_added + patch->lines_deleted;\n + \n + \tif (lines > state->max_change)\n + \t\tstate->max_change = lines;\n \n ## apply.h ##\n @@ apply.h: struct apply_state {\n @@ apply.h: struct apply_state {\n \t */\n -\tint max_change;\n -\tint max_len;\n -+\tunsigned max_change;\n ++\tunsigned int max_change;\n +\tsize_t max_len;\n \n \t/*\n \t * Records filenames that have been touched, in order to handle\n +@@ apply.h: struct patch {\n + \tint is_new, is_delete;\t/* -1 = unknown, 0 = false, 1 = true */\n + \tint rejected;\n + \tunsigned ws_rule;\n +-\tint lines_added, lines_deleted;\n ++\tunsigned int lines_added, lines_deleted;\n + \tint score;\n + \tint extension_linenr; /* first line specifying delete/new/rename/copy */\n + \tunsigned int is_toplevel_relative:1;\n2: 962ab421cf ! 2: b73f3e9fae apply: change some variables from `int` to `size_t`\n @@ Commit message\n \n some of which will trigger -Wsign-comparison warnings.\n \n - Change some of them to `size_t`/`unsigned`.\n + Change some of them to `size_t`/`unsigned int`.\n \n Signed-off-by: Zejun Zhao \n \n @@ apply.c: static void update_pre_post_images(struct image *preimage,\n \tconst char *fixed;\n \n \t/*\n +@@ apply.c: static int line_by_line_fuzzy_match(struct image *img,\n + \t\t\t\t struct image *postimage,\n + \t\t\t\t unsigned long current,\n + \t\t\t\t int current_lno,\n +-\t\t\t\t int preimage_limit)\n ++\t\t\t\t size_t preimage_limit)\n + {\n + \tint i;\n + \tsize_t imgoff = 0;\n @@ apply.c: static int match_fragment(struct apply_state *state,\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n @@ apply.c: static void update_image(struct apply_state *state,\n \n \t/*\n \t * If we are removing blank lines at the end of img,\n -@@ apply.c: static void summary_patch_list(struct patch *patch)\n - \n - static void patch_stats(struct apply_state *state, struct patch *patch)\n - {\n --\tint lines = patch->lines_added + patch->lines_deleted;\n -+\tunsigned lines = patch->lines_added + patch->lines_deleted;\n - \n +@@ apply.c: static void patch_stats(struct apply_state *state, struct patch *patch)\n \tif (lines > state->max_change)\n \t\tstate->max_change = lines;\n \tif (patch->old_name) {\n3: 09f42cbd5a = 3: 85938a18b1 apply: do a typecast to eliminate warnings\n4: 1142213871 = 4: ba31e9406d apply: cast some ptrdiff_t's to size_t's\n5: 8e27fb9c08 ! 5: 351e561843 apply: use `size_t` loop counters\n @@ apply.c: static void update_pre_post_images(struct image *preimage,\n \n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n @@ apply.c: static int line_by_line_fuzzy_match(struct image *img,\n - \t\t\t\t struct image *postimage,\n - \t\t\t\t unsigned long current,\n \t\t\t\t int current_lno,\n --\t\t\t\t int preimage_limit)\n -+\t\t\t\t size_t preimage_limit)\n + \t\t\t\t size_t preimage_limit)\n {\n -\tint i;\n +\tsize_t i;\n6: 60155b1aeb = 6: cdb3e806a1 apply: enable -Wsign-comparison checks\n-- \n2.43.0\n\n"},{"id":"512476","messageId":"20250216072843.72385-2-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 1/6] apply: correct type misuse in `apply.h`","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:38Z","receivedAt":"2025-02-16T07:29:17Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"There are a few `int` fields of structs in `apply.h` that are only used\nas unsigned integer and are only assigned unsigned integer value. Some\nof these misuse of `int` type would cause -Wsign-comparison warnings.\n\nFix this by\n\n - change `apply_state::max_change`'s type to `unsigned int` since it's\n just a counter of lines\n\n - change `apply_state::max_len`'s type to `size_t` since it's storing\n a length\n\n - change `patch::lines_added/deleted`'s types to `unsigned int` since\n they are just counters of lines\n\n - change the types of relevant variables in function `show_stats` and\n `patch_stats`\n\nNote that `printf`'s format string requires us to do some typecast\n(from `size_t` to `int`) on `max` in function `show_stats`. This is\nsafe because we already set a upper bound of `50` for `max` before the\ncast.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 11 ++++++-----\n apply.h | 6 +++---\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4a7b6120ac..4380a83386 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n {\n \tstruct strbuf qname = STRBUF_INIT;\n \tchar *cp = patch->new_name ? patch->new_name : patch->old_name;\n-\tint max, add, del;\n+\tsize_t max;\n+\tunsigned int add, del;\n \n \tquote_c_style(cp, &qname, NULL, 0);\n \n@@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n \t}\n \n \tif (patch->is_binary) {\n-\t\tprintf(\" %-*s | Bin\\n\", max, qname.buf);\n+\t\tprintf(\" %-*s | Bin\\n\", (int) max, qname.buf);\n \t\tstrbuf_release(&qname);\n \t\treturn;\n \t}\n \n-\tprintf(\" %-*s |\", max, qname.buf);\n+\tprintf(\" %-*s |\", (int) max, qname.buf);\n \tstrbuf_release(&qname);\n \n \t/*\n@@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch)\n \tdel = patch->lines_deleted;\n \n \tif (state->max_change > 0) {\n-\t\tint total = ((add + del) * max + state->max_change / 2) / state->max_change;\n+\t\tunsigned int total = ((add + del) * max + state->max_change / 2) / state->max_change;\n \t\tadd = (add * max + state->max_change / 2) / state->max_change;\n \t\tdel = total - add;\n \t}\n@@ -4287,7 +4288,7 @@ static void summary_patch_list(struct patch *patch)\n \n static void patch_stats(struct apply_state *state, struct patch *patch)\n {\n-\tint lines = patch->lines_added + patch->lines_deleted;\n+\tunsigned int lines = patch->lines_added + patch->lines_deleted;\n \n \tif (lines > state->max_change)\n \t\tstate->max_change = lines;\ndiff --git a/apply.h b/apply.h\nindex 90e887ec0e..8cd22756e2 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -90,8 +90,8 @@ struct apply_state {\n \t * we've seen, and the longest filename. That allows us to do simple\n \t * scaling.\n \t */\n-\tint max_change;\n-\tint max_len;\n+\tunsigned int max_change;\n+\tsize_t max_len;\n \n \t/*\n \t * Records filenames that have been touched, in order to handle\n@@ -127,7 +127,7 @@ struct patch {\n \tint is_new, is_delete;\t/* -1 = unknown, 0 = false, 1 = true */\n \tint rejected;\n \tunsigned ws_rule;\n-\tint lines_added, lines_deleted;\n+\tunsigned int lines_added, lines_deleted;\n \tint score;\n \tint extension_linenr; /* first line specifying delete/new/rename/copy */\n \tunsigned int is_toplevel_relative:1;\n-- \n2.43.0\n\n"},{"id":"512477","messageId":"20250216072843.72385-3-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 2/6] apply: change some variables from `int` to `size_t`","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:39Z","receivedAt":"2025-02-16T07:29:24Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Some assigned variables are mistyped as `int`, including\n\n - those whose values come from a system function returning `size_t`,\n\n - those that are used for array indexing,\n\n - those that represent length/size/distance,\n\nsome of which will trigger -Wsign-comparison warnings.\n\nChange some of them to `size_t`/`unsigned int`.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 21 +++++++++++----------\n 1 file changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4380a83386..4aa47a22b9 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,\n \t * and optional space with octal mode.\n \t */\n \tconst char *ptr, *eol;\n-\tint len;\n+\tsize_t len;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n \tptr = strchr(line, '.');\n@@ -2185,7 +2185,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t};\n \t\t\tint i;\n \t\t\tfor (i = 0; binhdr[i]; i++) {\n-\t\t\t\tint len = strlen(binhdr[i]);\n+\t\t\t\tsize_t len = strlen(binhdr[i]);\n \t\t\t\tif (len < size - hd &&\n \t\t\t\t !memcmp(binhdr[i], buffer + hd, len)) {\n \t\t\t\t\tstate->linenr++;\n@@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tstruct image fixed_preimage = IMAGE_INIT;\n \tsize_t insert_pos = 0;\n-\tint i, ctx, reduced;\n+\tint i, reduced;\n+\tsize_t ctx;\n \tconst char *fixed;\n \n \t/*\n@@ -2418,7 +2419,7 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t struct image *postimage,\n \t\t\t\t unsigned long current,\n \t\t\t\t int current_lno,\n-\t\t\t\t int preimage_limit)\n+\t\t\t\t size_t preimage_limit)\n {\n \tint i;\n \tsize_t imgoff = 0;\n@@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state,\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n \tsize_t fixed_len;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \tint ret;\n \n \tif (preimage->line_nr + current_lno <= img->line_nr) {\n@@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,\n {\n \tint i;\n \tunsigned long backwards, forwards, current;\n-\tint backwards_lno, forwards_lno, current_lno;\n+\tsize_t backwards_lno, forwards_lno, current_lno;\n \n \t/*\n \t * When running with --allow-overlap, it is possible that a hunk is\n@@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state,\n */\n static void update_image(struct apply_state *state,\n \t\t\t struct image *img,\n-\t\t\t int applied_pos,\n+\t\t\t size_t applied_pos,\n \t\t\t struct image *preimage,\n \t\t\t struct image *postimage)\n {\n@@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state,\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tsize_t result_alloc;\n \tchar *result;\n-\tint preimage_limit;\n+\tsize_t preimage_limit;\n \n \t/*\n \t * If we are removing blank lines at the end of img,\n@@ -4293,14 +4294,14 @@ static void patch_stats(struct apply_state *state, struct patch *patch)\n \tif (lines > state->max_change)\n \t\tstate->max_change = lines;\n \tif (patch->old_name) {\n-\t\tint len = quote_c_style(patch->old_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->old_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->old_name);\n \t\tif (len > state->max_len)\n \t\t\tstate->max_len = len;\n \t}\n \tif (patch->new_name) {\n-\t\tint len = quote_c_style(patch->new_name, NULL, NULL, 0);\n+\t\tsize_t len = quote_c_style(patch->new_name, NULL, NULL, 0);\n \t\tif (!len)\n \t\t\tlen = strlen(patch->new_name);\n \t\tif (len > state->max_len)\n-- \n2.43.0\n\n"},{"id":"512478","messageId":"20250216072843.72385-4-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 3/6] apply: do a typecast to eliminate warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:40Z","receivedAt":"2025-02-16T07:29:31Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"`git_hdr_len` is an `int` variable that can be negative and is used to\ncompare against a `len` of `size_t`, which will trigger\n-Wsign-comparison warnings\n\nCast `git_hdr_len` to `size_t` after an above-zero check.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4aa47a22b9..ac3e599bdf 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1592,7 +1592,7 @@ static int find_header(struct apply_state *state,\n \t\t\t\t\t\t\t\tsize, patch);\n \t\t\tif (git_hdr_len < 0)\n \t\t\t\treturn -128;\n-\t\t\tif (git_hdr_len <= len)\n+\t\t\tif ((size_t) git_hdr_len <= len)\n \t\t\t\tcontinue;\n \t\t\t*hdrsize = git_hdr_len;\n \t\t\treturn offset;\n-- \n2.43.0\n\n"},{"id":"512479","messageId":"20250216072843.72385-5-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 4/6] apply: cast some ptrdiff_t's to size_t's","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:41Z","receivedAt":"2025-02-16T07:29:37Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"There are several -Wsign-comparison warnings in \"apply.c\", complaining\nabout us comparing ptrdiff_t's with size_t's.\n\nFix these warnings by typecasting from ptrdiff_t to size_t. As to why\nthe casts is safe,\n\n - in function `date_len`, `date` is the starting address of a date at\n the end of the `line` and is guaranteed to be larger than (or equal\n to) `line`\n\n - in function `git_header_name`, `cp` is guaranteed to be larger than\n (or equal to) `second`, so `line + len` is greater than (or equal to)\n `cp` since we already treat `line + len - second` as a size_t\n\n - in function `git_header_name`, we are iterating `name` using\n `second`, so `second` is guaranteed to be greater than (or equal to)\n `name`\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex ac3e599bdf..c554a52f28 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -540,7 +540,7 @@ static size_t date_len(const char *line, size_t len)\n \t !isdigit(*p++) || !isdigit(*p++))\t/* Not a date. */\n \t\treturn 0;\n \n-\tif (date - line >= strlen(\"19\") &&\n+\tif ((size_t) (date - line) >= strlen(\"19\") &&\n \t isdigit(date[-1]) && isdigit(date[-2]))\t/* 4-digit year */\n \t\tdate -= strlen(\"19\");\n \n@@ -1207,7 +1207,7 @@ static char *git_header_name(int p_value,\n \t\tcp = skip_tree_prefix(p_value, second, line + llen - second);\n \t\tif (!cp)\n \t\t\tgoto free_and_fail1;\n-\t\tif (line + llen - cp != first.len ||\n+\t\tif ((size_t) (line + llen - cp) != first.len ||\n \t\t memcmp(first.buf, cp, first.len))\n \t\t\tgoto free_and_fail1;\n \t\treturn strbuf_detach(&first, NULL);\n@@ -1240,7 +1240,7 @@ static char *git_header_name(int p_value,\n \t\t\t\tgoto free_and_fail2;\n \n \t\t\tlen = sp.buf + sp.len - np;\n-\t\t\tif (len < second - name &&\n+\t\t\tif (len < (size_t) (second - name) &&\n \t\t\t !strncmp(np, name, len) &&\n \t\t\t isspace(name[len])) {\n \t\t\t\t/* Good */\n-- \n2.43.0\n\n"},{"id":"512480","messageId":"20250216072843.72385-6-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 5/6] apply: use `size_t` loop counters","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:42Z","receivedAt":"2025-02-16T07:29:42Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Some `int` loop counters trigger -Wsign-comparison warnings.\n\nUse `size_t` loop counters.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 30 +++++++++++++-----------------\n 1 file changed, 13 insertions(+), 17 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex c554a52f28..4c26f608ee 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1371,12 +1371,11 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t{ \"index \", gitdiff_index },\n \t\t\t{ \"\", gitdiff_unrecognized },\n \t\t};\n-\t\tint i;\n \n \t\tlen = linelen(line, size);\n \t\tif (!len || line[len-1] != '\\n')\n \t\t\tbreak;\n-\t\tfor (i = 0; i < ARRAY_SIZE(optable); i++) {\n+\t\tfor (size_t i = 0; i < ARRAY_SIZE(optable); i++) {\n \t\t\tconst struct opentry *p = optable + i;\n \t\t\tint oplen = strlen(p->str);\n \t\t\tint res;\n@@ -2097,7 +2096,6 @@ static void add_name_limit(struct apply_state *state,\n static int use_patch(struct apply_state *state, struct patch *p)\n {\n \tconst char *pathname = p->new_name ? p->new_name : p->old_name;\n-\tint i;\n \n \t/* Paths outside are not touched regardless of \"--include\" */\n \tif (state->prefix && *state->prefix) {\n@@ -2107,7 +2105,7 @@ static int use_patch(struct apply_state *state, struct patch *p)\n \t}\n \n \t/* See if it matches any of exclude/include rule */\n-\tfor (i = 0; i < state->limit_by_name.nr; i++) {\n+\tfor (size_t i = 0; i < state->limit_by_name.nr; i++) {\n \t\tstruct string_list_item *it = &state->limit_by_name.items[i];\n \t\tif (!wildmatch(it->string, pathname, 0))\n \t\t\treturn (it->util != NULL);\n@@ -2183,8 +2181,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \t\t\t\t\"Files \",\n \t\t\t\tNULL,\n \t\t\t};\n-\t\t\tint i;\n-\t\t\tfor (i = 0; binhdr[i]; i++) {\n+\t\t\tfor (size_t i = 0; binhdr[i]; i++) {\n \t\t\t\tsize_t len = strlen(binhdr[i]);\n \t\t\t\tif (len < size - hd &&\n \t\t\t\t !memcmp(binhdr[i], buffer + hd, len)) {\n@@ -2320,7 +2317,7 @@ static void update_pre_post_images(struct image *preimage,\n {\n \tstruct image fixed_preimage = IMAGE_INIT;\n \tsize_t insert_pos = 0;\n-\tint i, reduced;\n+\tint reduced;\n \tsize_t ctx;\n \tconst char *fixed;\n \n@@ -2330,7 +2327,7 @@ static void update_pre_post_images(struct image *preimage,\n \t * free \"oldlines\".\n \t */\n \timage_prepare(&fixed_preimage, buf, len, 1);\n-\tfor (i = 0; i < fixed_preimage.line_nr; i++)\n+\tfor (size_t i = 0; i < fixed_preimage.line_nr; i++)\n \t\tfixed_preimage.line[i].flag = preimage->line[i].flag;\n \timage_clear(preimage);\n \t*preimage = fixed_preimage;\n@@ -2339,7 +2336,7 @@ static void update_pre_post_images(struct image *preimage,\n \t/*\n \t * Adjust the common context lines in postimage.\n \t */\n-\tfor (i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n+\tfor (size_t i = reduced = ctx = 0; i < postimage->line_nr; i++) {\n \t\tsize_t l_len = postimage->line[i].len;\n \n \t\tif (!(postimage->line[i].flag & LINE_COMMON)) {\n@@ -2421,7 +2418,7 @@ static int line_by_line_fuzzy_match(struct image *img,\n \t\t\t\t int current_lno,\n \t\t\t\t size_t preimage_limit)\n {\n-\tint i;\n+\tsize_t i;\n \tsize_t imgoff = 0;\n \tsize_t preoff = 0;\n \tsize_t extra_chars;\n@@ -2488,7 +2485,7 @@ static int match_fragment(struct apply_state *state,\n \t\t\t unsigned ws_rule,\n \t\t\t int match_beginning, int match_end)\n {\n-\tint i;\n+\tsize_t i;\n \tconst char *orig, *target;\n \tstruct strbuf fixed = STRBUF_INIT;\n \tchar *fixed_buf;\n@@ -2665,12 +2662,11 @@ static int match_fragment(struct apply_state *state,\n \tfor ( ; i < preimage->line_nr; i++) {\n \t\tsize_t fixstart = fixed.len; /* start of the fixed preimage */\n \t\tsize_t oldlen = preimage->line[i].len;\n-\t\tint j;\n \n \t\t/* Try fixing the line in the preimage */\n \t\tws_fix_copy(&fixed, orig, oldlen, ws_rule, NULL);\n \n-\t\tfor (j = fixstart; j < fixed.len; j++) {\n+\t\tfor (size_t j = fixstart; j < fixed.len; j++) {\n \t\t\tif (!isspace(fixed.buf[j])) {\n \t\t\t\tret = 0;\n \t\t\t\tgoto out;\n@@ -2800,7 +2796,7 @@ static void update_image(struct apply_state *state,\n \t * remove the copy of preimage at offset in img\n \t * and replace it with postimage\n \t */\n-\tint i, nr;\n+\tint nr;\n \tsize_t remove_count, insert_count, applied_at = 0;\n \tsize_t result_alloc;\n \tchar *result;\n@@ -2819,11 +2815,11 @@ static void update_image(struct apply_state *state,\n \tif (preimage_limit > img->line_nr - applied_pos)\n \t\tpreimage_limit = img->line_nr - applied_pos;\n \n-\tfor (i = 0; i < applied_pos; i++)\n+\tfor (size_t i = 0; i < applied_pos; i++)\n \t\tapplied_at += img->line[i].len;\n \n \tremove_count = 0;\n-\tfor (i = 0; i < preimage_limit; i++)\n+\tfor (size_t i = 0; i < preimage_limit; i++)\n \t\tremove_count += img->line[applied_pos + i].len;\n \tinsert_count = postimage->buf.len;\n \n@@ -2852,7 +2848,7 @@ static void update_image(struct apply_state *state,\n \t\t\t img->line_nr - (applied_pos + preimage_limit));\n \tCOPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);\n \tif (!state->allow_overlap)\n-\t\tfor (i = 0; i < postimage->line_nr; i++)\n+\t\tfor (size_t i = 0; i < postimage->line_nr; i++)\n \t\t\timg->line[applied_pos + i].flag |= LINE_PATCHED;\n \timg->line_nr = nr;\n }\n-- \n2.43.0\n\n"},{"id":"512481","messageId":"20250216072843.72385-7-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"20250216072843.72385-1-jelly.zhao.42@gmail.com","subject":"[PATCH v3 6/6] apply: enable -Wsign-comparison checks","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-16T07:28:43Z","receivedAt":"2025-02-16T07:29:47Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Remove the `#define DISABLE_SIGN_COMPARE_WARNINGS` header line.\n\nSigned-off-by: Zejun Zhao \n---\n apply.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 4c26f608ee..f1113d830d 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -8,7 +8,6 @@\n */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"abspath.h\"\n-- \n2.43.0\n\n"},{"id":"512626","messageId":"xmqqpljfxhvp.fsf@gitster.g","threadId":"62902","inReplyTo":"20250216071202.61372-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-18T17:49:46Z","receivedAt":"2025-02-18T17:49:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zejun Zhao writes:\n\n> On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote,\n>> > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,\n>> > {\n>> > \tint i;\n>> > \tunsigned long backwards, forwards, current;\n>> > -\tint backwards_lno, forwards_lno, current_lno;\n>> > +\tsize_t backwards_lno, forwards_lno, current_lno;\n>> > \n>> > \t/*\n>> > \t * When running with --allow-overlap, it is possible that a hunk is\n>> \n>> These are a bit curious, as they store `line`, which is itself an `int`\n>> parameter. As far as I understand, the only caller is also only ever\n>> passing a positive integer here.\n>\n> They do store an `int` parameter, which is `line`. However, we cannot change \n> `line` to unsigned since it can temporarily store an negative value before \n> the assignments to these `*_lno` happen.\n\nCorrect. Doesn't it mean that a change that makes these line-number\nvariables to size_t is wrong? Of course the change is not made to\nbreak the code but may be to please some code paths in other parts\nof the system that wants these line-number variables that are\ncurrently \"int\" to compare or assign without range-checking with\n\"size_t\" quantity or variable, but then shouldn't we fix these\nplaces, not the types of these line-number variables that use the\nplatform natural integers?\n\nTHanks.\n"},{"id":"512788","messageId":"20250221063705.15539-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"xmqqpljfxhvp.fsf@gitster.g","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-21T06:37:05Z","receivedAt":"2025-02-21T06:38:14Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"On Tue, Feb 18 2025 09:49:46 -0800, Junio C Hamano wrote,\n> Doesn't it mean that a change that makes these line-number\n> variables to size_t is wrong? Of course the change is not made to\n> break the code but may be to please some code paths in other parts\n> of the system that wants these line-number variables that are\n> currently \"int\" to compare or assign without range-checking with\n> \"size_t\" quantity or variable,\n\nSorry but I don't think the change here is wrong. These line-number variables \nare used as unsigned integers (compared with unsigned, used to index arrays, \n...). And what's more important, we even do range-checking on `line` before we \nassign it to those line-number's, which means we want to ensure the \nline-number's always store an unsigned integer. So why not declare these \nline-number's as unsigned if we initialize them as unsigned and at the same \ntime use them as unsigned?\n\nRegards.\n"},{"id":"512834","messageId":"xmqq34g7cjf2.fsf@gitster.g","threadId":"62902","inReplyTo":"20250221063705.15539-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-21T17:11:13Z","receivedAt":"2025-02-21T17:11:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zejun Zhao writes:\n\n> On Tue, Feb 18 2025 09:49:46 -0800, Junio C Hamano wrote,\n>> Doesn't it mean that a change that makes these line-number\n>> variables to size_t is wrong? Of course the change is not made to\n>> break the code but may be to please some code paths in other parts\n>> of the system that wants these line-number variables that are\n>> currently \"int\" to compare or assign without range-checking with\n>> \"size_t\" quantity or variable,\n>\n> Sorry but I don't think the change here is wrong. These line-number variables \n> are used as unsigned integers (compared with unsigned, used to index arrays, \n> ...).\n\nI thought that we made it \"int\" not \"unsigned\" as functions that\ncompute these line numbers needed to signal an error \"oh the input\nis not a valid line number\" and one of the most natural ways in C to\ndo so was to return a negative values from them. Isn't that what is\nhappening?\n\nFor that kind of use, the storage (i.e. variables and structure\nmembers) does not have to be signed as long as we check the error\nreturns from these functions (and I think we do), but then compilers\nmay give you needress warnings when you do assign such a value\nreturned from the function to the final storage after checking the\nvalidity of the value. You may use casts but that is trading clean\ncode for clean compilation without complaint from needless warnings.\n\nOr you can make the storage also signed and we do not have to worry\nabout squelching the warnings with ugly casts. When we know we are\nnot handling patches with a billion of lines, losing half the range\nby going signed is rather a no-brainer choice between the two.\n\nI do not see any good justification, even if we buy \"the final\nstorage should be unsigned, since we always check before assignment\"\n(which I am not too opposed to), for using size_t instead of\nplatform natural integer (be it signed or unsigned) for the line\nnumbers, though.\n\nThanks.\n\n"},{"id":"512883","messageId":"20250223173629.16489-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"xmqq34g7cjf2.fsf@gitster.g","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-23T17:36:28Z","receivedAt":"2025-02-23T17:37:10Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Thank you for clarifying. Now I can see the concern on whether the whole bunch \nof things worth it; it's a trade-off between cleanness and robustness of the \ncodebase and it's very common due to some coding conventions (e.g. error \nhandling) in C. Maybe in this case we should just leave the working code work.\n\nNow I'd like to ask for your advice on this issue, which I may be supposed to \ndo before sending any actual patches, should I push forward this patchset \nfurther or pick another microproject?\n\nThanks.\n"},{"id":"512897","messageId":"xmqqv7sz76zl.fsf@gitster.g","threadId":"62902","inReplyTo":"20250223173629.16489-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-24T14:27:42Z","receivedAt":"2025-02-24T14:27:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zejun Zhao writes:\n\n> Now I'd like to ask for your advice on this issue, which I may be supposed to \n> do before sending any actual patches, should I push forward this patchset \n> further or pick another microproject?\n\nIt really depends on what \"this patchset\" you mean.\n\nI do think that there are still places in our code base where we\ntruncate size_t values that we eventually use for allocation by\nmistakenly mix arithmetic with smaller type, and I do think it is\nworth finding and fixing them. So a patch that fixes a code path\nwith such an issue would still be a nice thing to do. If there such\na change (I do not offhand recall) in the 6 patches from you, that\npart of the series may want to be resurrected; but I do not think\nchanging \"int\" for line numbers to \"size_t\" is one of such changes.\n\nIf you mean \"find anything that -Wsign-compare warns about and\nsquelch the warning by using widest type common among the quantities\ninvolved in the expression the compiler warns about\", I do not think\nit is a good idea [*1*].\n\n\n[Reference]\n\n*1* https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n"},{"id":"512941","messageId":"20250225032444.9901-1-jelly.zhao.42@gmail.com","threadId":"62902","inReplyTo":"xmqqv7sz76zl.fsf@gitster.g","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Zejun Zhao","fromEmail":"jelly.zhao.42@gmail.com","sentAt":"2025-02-25T03:24:44Z","receivedAt":"2025-02-25T03:25:41Z","isPatch":true,"sender":{"key":"jelly.zhao.42@gmail.com","avatar":null},"body":"Thank you for explaining. I'll separate the desired changes from this patch to \nanother since the topic will be different.\n\nBy the way, should we remove the idea from the microproject list now that we \nmay not want it?\n"},{"id":"513010","messageId":"xmqqseo2kwxj.fsf@gitster.g","threadId":"62902","inReplyTo":"20250225032444.9901-1-jelly.zhao.42@gmail.com","subject":"Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-25T12:53:28Z","receivedAt":"2025-02-25T12:53:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zejun Zhao writes:\n\n> Thank you for explaining. I'll separate the desired changes from this patch to \n> another since the topic will be different.\n>\n> By the way, should we remove the idea from the microproject list now that we \n> may not want it?\n\nIt is not like we do not want it, I think.\n\nIf you read https://git.github.io/SoC-2025-Microprojects/ (the first\nentry) carefully, it does not say \"find anything that -Wsign-compare\nwarns about and squelch the warning by using widest type common\namong the quantities involved in the expression the compiler warns\nabout\" at all. Most specifically, it does *not* tell you to achieve\nit \"by using widest type\". You would squelch the warning using the\nmost appropriate type, which may not be the widest one. In the case\nwe discussed in this thread, it was line number whose type was the\nplatform natural integer, but mixed up with other size_t things that\ncaused the compiler warning. Blindly using size_t to squelch it may\nnot be what we want, but that does not mean we do not want to\nsquelch it by doing something else (like, by not bringing \"size_t\"\nquantity into the picture).\n"}]}