Re: [GSOC][PATCH v2 2/6] apply: change some variables from `int` to `size_t`

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 13, 2025, 06:08 UTC
Message-ID: <Z62MTZXQD3Wa47Jz@pks.im>
In-Reply-To: <20250209081216.241350-3-jelly.zhao.42@gmail.com>

On Sun, Feb 09, 2025 at 08:12:12AM +0000, Zejun Zhao wrote:

Show 30 quoted lines

> Some assigned variables are mistyped as `int`, including
> 
>   - those whose values come from a system function returning `size_t`,
> 
>   - those that are used for array indexing,
> 
>   - those that represent length/size/distance,
> 
> some of which will trigger -Wsign-comparison warnings.
> 
> Change some of them to `size_t`/`unsigned`.
> 
> Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com>
> ---
>  apply.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index 831b338155..b4ae74a5fb 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,
>  	 * and optional space with octal mode.
>  	 */
>  	const char *ptr, *eol;
> -	int len;
> +	size_t len;
>  	const unsigned hexsz = the_hash_algo->hexsz;
>  
>  	ptr = strchr(line, '.');

This is storing the result of `ptr - line`, which will be a positive integer. It's later passed to `memcpy()`, which expects a `size_t`.

Show 10 quoted lines

> @@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage,
>  {
>  	struct image fixed_preimage = IMAGE_INIT;
>  	size_t insert_pos = 0;
> -	int i, ctx, reduced;
> +	int i, reduced;
> +	size_t ctx;
>  	const char *fixed;
>  
>  	/*

`ctx` is indexing an array and counts against `preimage->line_nr`, which is a `size_t`, too.

Show 9 quoted lines

> @@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state,
>  	struct strbuf fixed = STRBUF_INIT;
>  	char *fixed_buf;
>  	size_t fixed_len;
> -	int preimage_limit;
> +	size_t preimage_limit;
>  	int ret;
>  
>  	if (preimage->line_nr + current_lno <= img->line_nr) {

This one stores `struct image::line_nr`, which is a `size_t`.

Show 9 quoted lines

> @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,
>  {
>  	int i;
>  	unsigned long backwards, forwards, current;
> -	int backwards_lno, forwards_lno, current_lno;
> +	size_t backwards_lno, forwards_lno, current_lno;
>  
>  	/*
>  	 * When running with --allow-overlap, it is possible that a hunk is

These are a bit curious, as they store `line`, which is itself an `int` parameter. As far as I understand, the only caller is also only ever passing a positive integer here.

Show 18 quoted lines

> @@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state,
>   */
>  static void update_image(struct apply_state *state,
>  			 struct image *img,
> -			 int applied_pos,
> +			 size_t applied_pos,
>  			 struct image *preimage,
>  			 struct image *postimage)
>  {
> @@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state,
>  	size_t remove_count, insert_count, applied_at = 0;
>  	size_t result_alloc;
>  	char *result;
> -	int preimage_limit;
> +	size_t preimage_limit;
>  
>  	/*
>  	 * If we are removing blank lines at the end of img,

The caller makes sure that the function only gets called when the parameter is a positive integer.

Show 6 quoted lines

> @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch)
>  
>  static void patch_stats(struct apply_state *state, struct patch *patch)
>  {
> -	int lines = patch->lines_added + patch->lines_deleted;
> +	unsigned lines = patch->lines_added + patch->lines_deleted;

This one is curious again, as the type of these variables is an `int`. This should likely be adapted in tandem if they cannot be negative.

Patrick