Re: [GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 13, 2025, 09:51 UTC
Message-ID: <CAOLa=ZQbVBmGnsvWUz0ByW8z5NMgLY2oZZ1RLJeoWJST9PGAvQ@mail.gmail.com>
In-Reply-To: <20250209081216.241350-2-jelly.zhao.42@gmail.com>

Zejun Zhao <jelly.zhao.42@gmail.com> writes:

Show 7 quoted lines

> `.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.

Show 25 quoted lines

>   - 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@gmail.com>
> ---
>  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.

Show 43 quoted lines

>  	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.