Re: [PATCH v2 12/14] builtin/patch-id: fix type of `get_one_patchid()`
- From
shejialuo <shejialuo@gmail.com>
- Date
- Dec 2, 2024, 13:18 UTC
- Message-ID
- <Z02zq7Wc1A2ot-Oe@ArchLinux>
- In-Reply-To
- <20241202-pks-sign-compare-v2-12-e7f0ad92a749@pks.im>
On Mon, Dec 02, 2024 at 01:04:44PM +0100, Patrick Steinhardt wrote:
> In `get_one_patchid()` we assign either the result of `strlen()` or > `remove_space()` to `len`. But while the former correctly returns a > `size_t`, the latter returns an `int` to indicate the length of the
It is a little misleading about the statement "the latte returns an `int` to indicate the length of the string".
Should "without spaces" be appended into the last? However, don't worth a reroll.
Show 38 quoted lines
> string even though it cannot ever return a negative value. This causes a
> warning with "-Wsign-conversion".
>
> In fact, even `get_one_patchid()` itself is also using an integer as
> return value even though it always returns the length of the patch, and
> this bubbles up to other callers.
>
> Adapt the function and its helpers to use `size_t` for string lengths
> consistently.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/patch-id.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 56585757477911c96bbb9ef2cf3710691b8e744e..f540d8daa736b027649c8c64ffe5100cf4044037 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -1,5 +1,4 @@
> #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
> #include "builtin.h"
> #include "config.h"
> @@ -10,13 +9,13 @@
> #include "parse-options.h"
> #include "setup.h"
>
> -static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)
> +static void flush_current_id(size_t patchlen, struct object_id *id, struct object_id *result)
> {
> if (patchlen)
> printf("%s %s\n", oid_to_hex(result), oid_to_hex(id));
> }
>
> -static int remove_space(char *line)
> +static size_t remove_space(char *line)So, eventually, we have decided to change the return type from "int" to "size_t". This avoids casting "int" to "size_t", which is good. And it would be more natural, because "remove_space" never returns negative result.
> {
> char *src = line;
> char *dst = line;Thanks, Jialuo