On Fri, Nov 29, 2024 at 02:13:27PM +0100, Patrick Steinhardt wrote:
> We have several cases in our codebase where the ternary operator changes
> signedness from a signed integer type to an unsigned one. This causes
> warnings with `-Wsign-compare`. Generally, we seem to have three classes
> of this in our codebase:
>
> - Cases where we know that the result is well-formed, e.g. when
> indexing into strings to determine the length of substrings.
>
> - Cases where we want `-1` to mean "unlimited", counting on the
> wrap-around.
>
> - Cases where we may indeed run into problems when one of the
> statements returns a value that is too big.
>
> Out of these only the last class is a bit worrying, but we can address
> it by adding a call to `cast_size_t_to_int()`. Like this we're better
> protected in case we have unexpectedly huge input as we'd die instead of
> silently doing the wrong thing.
>
That's good, without using "case_size_t_to_int", we rely on the compiler
to do the conversion, which is bad. Now we will die the program.
[snip]
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 56585757477911c96bbb9ef2cf3710691b8e744e..87fa586c4d552ba61cd2ac2cf079d68241eb3275 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -163,7 +163,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
> after--;
>
> /* Add line to hash algo (possibly removing whitespace) */
> - len = verbatim ? strlen(line) : remove_space(line);
> + len = verbatim ? cast_size_t_to_int(strlen(line)) : remove_space(line);
Here, we convert the unsigned "strelen(line)" because the `len` is a
signed value. In the first glance, I think we should change the return
type of "remove_space" to "size_t", because it will always return the
unsigned value.
However, in our codebase, we use "int" as the return value in the most
situations. So, this change make senses.
> patchlen += len;
> the_hash_algo->update_fn(&ctx, line, len);
> }
> diff --git a/gpg-interface.c b/gpg-interface.c
> index a67d80475bf9d8452de0c3ae9bb08ceeb4c11c4b..e1720361f17e8b3b3315f0a5d93a827e11b2b036 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -700,7 +700,7 @@ size_t parse_signed_buffer(const char *buf, size_t size)
> match = len;
>
> eol = memchr(buf + len, '\n', size - len);
> - len += eol ? eol - (buf + len) + 1 : size - len;
> + len += eol ? (size_t) (eol - (buf + len) + 1) : size - len;
When reading the code, I wonder what if "eol - (buf + len) + 1" is less
than zero. If so, we would cause underflow. We have created a helper
function "cast_size_t_to_int". Do we need to create a function to safely
convert the potential signed integer to "size_t"?
> diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
> index ea9b9b656307d32bdc1f2e15a91793b1dda9c463..31dbe7db4ac61639541f15d262cea64368fec78f 100644
> --- a/t/helper/test-csprng.c
> +++ b/t/helper/test-csprng.c
> @@ -1,5 +1,3 @@
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
> #include "test-tool.h"
> #include "git-compat-util.h"
>
> @@ -14,7 +12,7 @@ int cmd__csprng(int argc, const char **argv)
> return 2;
> }
>
> - count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
> + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : (unsigned long) -1L;
Here we use "-1" to represent the unlimited, and the type of `count` is
unsigned long. So we need to explicitly case the type of "-1L". But this
is strange, why not just use the "ULONG_MAX" directly?
>
> while (count) {
> unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
> diff --git a/t/helper/test-genrandom.c b/t/helper/test-genrandom.c
> index 5b51e6648d8e698b09f400efcf67a0708c226e9d..efca20e7efff46bf66c2b8888ce88db02e545cd5 100644
> --- a/t/helper/test-genrandom.c
> +++ b/t/helper/test-genrandom.c
> @@ -4,8 +4,6 @@
> * Copyright (C) 2007 by Nicolas Pitre, licensed under the GPL version 2.
> */
>
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
> #include "test-tool.h"
> #include "git-compat-util.h"
>
> @@ -24,7 +22,7 @@ int cmd__genrandom(int argc, const char **argv)
> next = next * 11 + *c;
> } while (*c++);
>
> - count = (argc == 3) ? strtoul(argv[2], NULL, 0) : -1L;
> + count = (argc == 3) ? strtoul(argv[2], NULL, 0) : (unsigned long) -1L;
>
Similar with the above comment.
> while (count--) {
> next = next * 1103515245 + 12345;
>
> --
> 2.47.0.366.g5daf58cba8.dirty
>
Thanks,
Jialuo
