Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 5, 2025, 07:47 UTC
- Message-ID
- <Z6MXevArKhRLacAb@pks.im>
- In-Reply-To
- <20250205014055.737190-1-jelly.zhao.42@gmail.com>
On Wed, Feb 05, 2025 at 01:40:55AM +0000, Zejun Zhao wrote:
Show 21 quoted lines
> There are several -Wsign-comparison warnings in "apply.c", which can be > classified into the following three types: > > 1. comparing a `length` of `size_t` type with a result of pointer > arithmetic of `int` type > > 2. comparing a `length` of `size_t` type with a `length` of `int` type > > 3. comparing a loop count `i` of `int` type with an unsigned loop > bound > > Fix these warnings following one basic principle: do not touch the > relevant logics and keep the behaviors of the code. Adopt three > different strategies for each of the above three types: > > 1. cast the result of pointer arithmetic to `size_t` type > > 2. try to change the type of the `length` to `size_t` (may fall back > to Strategy 1 if the variable is not guaranteed to be unsigned) > > 3. use a loop count `i` of `size_t` type
You should split up this patch into a series, as it is really hard to follow what's going on. There are a couple of things happening:
- You change types in `struct apply_state`, which bubbles up.
- You adapt `git_hdr_len()` to receive different inputs, which bubbles
up.- You perform small fixes in several places.
It might also be a good idea to split out the loop counters into a separate commit, as those are trivially correct.
Show 24 quoted lines
> Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com>
> ---
> apply.c | 73 +++++++++++++++++++++++++++------------------------------
> apply.h | 6 ++---
> 2 files changed, 38 insertions(+), 41 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 4a7b6120ac..0c7b89dc3a 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -8,7 +8,6 @@
> */
>
> #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
> #include "git-compat-util.h"
> #include "abspath.h"
> @@ -540,7 +539,7 @@ static size_t date_len(const char *line, size_t len)
> !isdigit(*p++) || !isdigit(*p++)) /* Not a date. */
> return 0;
>
> - if (date - line >= strlen("19") &&
> + if ((size_t) (date - line) >= strlen("19") &&We know that `date` is always bigger than or equal to `line`, so this is correct.
Show 8 quoted lines
> @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state, > * and optional space with octal mode. > */ > const char *ptr, *eol; > - int len; > - const unsigned hexsz = the_hash_algo->hexsz; > + size_t len; > + const size_t hexsz = the_hash_algo->hexsz;
The change to `hexsz` shouldn't be needed, even if it makes us match the type of `hexsz` as declared in `git_hash_algo`.
> ptr = strchr(line, '.'); > - if (!ptr || ptr[1] != '.' || hexsz < ptr - line) > + if (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))
`ptr` is the reline of `strrchr(line)`, so it must be either `NULL` or greater than or equal to `line`, so this cast is fine.
Show 9 quoted lines
> @@ -1158,7 +1157,7 @@ static const char *skip_tree_prefix(int p_value,
> */
> static char *git_header_name(int p_value,
> const char *line,
> - int llen)
> + size_t llen)
> {
> const char *name;
> const char *second = NULL;It would make sense to split this change out into a separate commit, as it bubbles up into calling functions, as well.
Show 9 quoted lines
> @@ -1207,7 +1206,7 @@ static char *git_header_name(int p_value, > cp = skip_tree_prefix(p_value, second, line + llen - second); > if (!cp) > goto free_and_fail1; > - if (line + llen - cp != first.len || > + if ((size_t) (line + llen - cp) != first.len || > memcmp(first.buf, cp, first.len)) > goto free_and_fail1; > return strbuf_detach(&first, NULL);
This cast should be fine, too.
Show 9 quoted lines
> @@ -1240,7 +1239,7 @@ static char *git_header_name(int p_value,
> goto free_and_fail2;
>
> len = sp.buf + sp.len - np;
> - if (len < second - name &&
> + if (len < (size_t) (second - name) &&
> !strncmp(np, name, len) &&
> isspace(name[len])) {
> /* Good */This one, too. `second` is iterating through `name`, so it's always greater or equal to `name`.
Show 17 quoted lines
> @@ -1371,14 +1370,13 @@ int parse_git_diff_header(struct strbuf *root,
> { "index ", gitdiff_index },
> { "", gitdiff_unrecognized },
> };
> - int i;
>
> len = linelen(line, size);
> if (!len || line[len-1] != '\n')
> break;
> - for (i = 0; i < ARRAY_SIZE(optable); i++) {
> + for (size_t i = 0; i < ARRAY_SIZE(optable); i++) {
> const struct opentry *p = optable + i;
> - int oplen = strlen(p->str);
> + size_t oplen = strlen(p->str);
> int res;
> if (len < oplen || memcmp(p->str, line, oplen))
> continue;Makes sense.
Show 9 quoted lines
> @@ -1592,7 +1590,7 @@ static int find_header(struct apply_state *state, > size, patch); > if (git_hdr_len < 0) > return -128; > - if (git_hdr_len <= len) > + if ((size_t) git_hdr_len <= len) > continue; > *hdrsize = git_hdr_len; > return offset;
We've asserted that `git_hdr_len` isn't negative, so the cast is fine.
> @@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si > }; > int i;
This may arguably be `size_t`, as well.
Show 15 quoted lines
> @@ -2257,12 +2255,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)
> }
>
> if (patch->is_binary) {
> - printf(" %-*s | Bin\n", max, qname.buf);
> + printf(" %-*s | Bin\n", (int) max, qname.buf);
> strbuf_release(&qname);
> return;
> }
>
> - printf(" %-*s |", max, qname.buf);
> + printf(" %-*s |", (int) max, qname.buf);
> strbuf_release(&qname);
>
> /*Do we _know_ that `max` fits into an `int`?
Patrick