Re: [PATCH 08/10] global: trivial conversions to fix `-Wsign-compare` warnings

From: Jeff King <peff@peff.net>
Date: Dec 1, 2024, 22:07 UTC
Message-ID: <20241201220712.GE145938@coredump.intra.peff.net>
In-Reply-To: <20241129-pks-sign-compare-v1-8-fc406b984bc9@pks.im>

On Fri, Nov 29, 2024 at 02:13:29PM +0100, Patrick Steinhardt wrote:

> We have a bunch of loops all over the place which iterate through a
> signed integer with an unsigned index, which generates warnings. Address

I had trouble parsing this first sentence. Did you mean "max" or "bound" instead of "index"? I'd have also thought that "with" is the iteration variable, not the boundary. So:

  ...which iterate up to an unsigned boundary using a signed index (and
  thus comparing the signed and unsigned values in the loop condition).

or something?

Show 11 quoted lines

> @@ -195,9 +192,7 @@ int git_default_advice_config(const char *var, const char *value)
>  
>  void list_config_advices(struct string_list *list, const char *prefix)
>  {
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(advice_setting); i++)
> +	for (size_t i = 0; i < ARRAY_SIZE(advice_setting); i++)
>  		list_config_item(list, prefix, advice_setting[i].key);
>  }
>

I didn't exhaustively stare at each one, but skimmed through and they all look like obvious improvements, like this.

I do note that you tend to move the declaration of the variable to scope it to the loop. I think that's OK (and in general is a good practice), but two thoughts:

  1. Are we all-in on C99 loop scoping like this? I wasn't sure if we
     were still in the weather-balloon period (but I admit I have not
     paid close attention, so this is really just double-checking).

  2. The compiler should mostly catch any cases where this is not
     correct to do (because any references to the variable outside the
     loop would now be errors). The exception is if "i" here were
     shadowing another variable. That seems pretty unlikely, though.

-Peff