Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings

From: Zejun Zhao <jelly.zhao.42@gmail.com>
Date: Feb 16, 2025, 07:12 UTC
Message-ID: <20250216071202.61372-1-jelly.zhao.42@gmail.com>
In-Reply-To: <Z62MTZXQD3Wa47Jz@pks.im>

On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote,

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

They do store an `int` parameter, which is `line`. However, we cannot change `line` to unsigned since it can temporarily store an negative value before the assignments to these `*_lno` happen. Below are from function `find_pos` (without any change).

  /*
   * If match_beginning or match_end is specified, there is no
   * point starting from a wrong line that will never match and
   * wander around and wait for a match at the specified end.
   */
  if (match_beginning)
	  line = 0;
  else if (match_end)
	  line = img->line_nr - preimage->line_nr;

  /*
   * Because the comparison is unsigned, the following test
   * will also take care of a negative line number that can
   * result when match_end and preimage is larger than the target.
   */
  if (line > img->line_nr)
	  line = img->line_nr;

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

Thank you for identifying. Will be in next version.