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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 2, 2024, 07:54 UTC
Message-ID: <Z01nnsh9vKJi-NJP@pks.im>
In-Reply-To: <20241201220712.GE145938@coredump.intra.peff.net>

On Sun, Dec 01, 2024 at 05:07:12PM -0500, Jeff King wrote:

Show 13 quoted lines

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

That reads much better, thanks.

Show 22 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).

Yeah, this should be safe nowadays:

    $ git grep 'for (int ' | wc -l
    36
    $ git grep 'for (unsigned ' | wc -l
    1
    $ git grep 'for (size_t ' | wc -l
    105

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

I've been on the lookout for such patterns and didn't spot any.

Patrick