On Sun, Dec 01, 2024 at 05:07:12PM -0500, Jeff King wrote:
> 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.
> > @@ -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
