Zejun Zhao <jelly.zhao.42@xxxxxxxxx> writes:
> `.max_change` and `.max_len` of `apply_state` are only used as unsigned
> integers. Misuse of `int` type would cause -Wsign-comparison warnings.
>
> Fix this by
>
> - change `.max_change`'s type to `unsigned` since it's just a counter
>
Looking at `.max_change` it seems like this is only assigned in
`patch_stats()` where we do
int lines = patch->lines_added + patch->lines_deleted;
if (lines > state->max_change)
state->max_change = lines;
In this case shouldn't we first convert `.lines_added` `.lines_deleted`
to also be 'unsigned int' in the first place?
> - change `.max_len`'s type to `size_t` since it's a length
>
I see that this is assigned the return value of `strlen()` so this
makes sense.
> - change the types of relevant variables in function `show_stats`
>
> Note that `printf`'s format string requires us to do some typecast
> (from `size_t` to `int`) on `max` in function `show_stats`. This is
> safe because we already set a upper bound of `50` for `max` before the
> cast.
>
> Signed-off-by: Zejun Zhao <jelly.zhao.42@xxxxxxxxx>
> ---
> apply.c | 9 +++++----
> apply.h | 4 ++--
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 4a7b6120ac..831b338155 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch)
> {
> struct strbuf qname = STRBUF_INIT;
> char *cp = patch->new_name ? patch->new_name : patch->old_name;
> - int max, add, del;
> + size_t max;
> + unsigned add, del;
>
Tangential to this patch, I don't think we have a guideline on using
'unsigned' vs 'unsigned int'. Probably we should.
> quote_c_style(cp, &qname, NULL, 0);
>
> @@ -2257,12 +2258,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);
>
> /*
> @@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch)
> del = patch->lines_deleted;
>
> if (state->max_change > 0) {
> - int total = ((add + del) * max + state->max_change / 2) / state->max_change;
> + unsigned total = ((add + del) * max + state->max_change / 2) / state->max_change;
> add = (add * max + state->max_change / 2) / state->max_change;
> del = total - add;
> }
> diff --git a/apply.h b/apply.h
> index 90e887ec0e..f7f369d44f 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -90,8 +90,8 @@ struct apply_state {
> * we've seen, and the longest filename. That allows us to do simple
> * scaling.
> */
> - int max_change;
> - int max_len;
> + unsigned max_change;
> + size_t max_len;
>
> /*
> * Records filenames that have been touched, in order to handle
> --
> 2.43.0
The rest look good.
Attachment:
signature.asc
Description: PGP signature
