Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 5, 2025, 12:58 UTC
- Message-ID
- <xmqqh658r1im.fsf@gitster.g>
- In-Reply-To
- <20250205014055.737190-1-jelly.zhao.42@gmail.com>
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;
I thought that I already saw this discussed in another thread.
The .hexsz of any hash algorithm would never be larger than what a platform natural "unsigned" integer type can hold, so using size_t for the member _is_ the wrong thing to do and the fix may be the other way around, no?
There are genuinely good changes (correcting assigned variable from int to size_t when the value ultimately came from a system function that returns size_t) in this patch, but there are other annoying ones like these, so I am not sure.
> ptr = strchr(line, '.'); > - if (!ptr || ptr[1] != '.' || hexsz < ptr - line) > + if (!ptr || ptr[1] != '.' || hexsz < (size_t) (ptr - line))
Is this about -Wsign-compare complaining about size_t vs ptrdiff_t?
Show 6 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 ||
Ditto.
> - if (len < second - name && > + if (len < (size_t) (second - name) &&
Ditto.
I said this before, but I am not sure if being strict about "-Wsign-compare" is really buying us much. If we are getting so many false positives that need to be squelched by churning the code with so many typecasts in order to find a new and real problem, is it really worth it?