On Wed, Feb 05, 2025 at 01:40:55AM +0000, Zejun Zhao wrote:
> 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.
> Signed-off-by: Zejun Zhao <jelly.zhao.42@xxxxxxxxx>
> ---
> 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.
> @@ -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.
> @@ -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.
> @@ -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.
> @@ -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`.
> @@ -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.
> @@ -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.
> @@ -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
