Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings
- From
- Zejun Zhao <jelly.zhao.42@gmail.com>
- Date
- Feb 5, 2025, 15:42 UTC
- Message-ID
- <20250205154232.237380-1-jelly.zhao.42@gmail.com>
- In-Reply-To
- <Z6MXevArKhRLacAb@pks.im>
Thank you for reviewing this patch.
On Wed, Feb 05, 2025 08:47:06 +0100, Patrick Steinhardt wrote:
Show 12 quoted lines
> You should split up this patch into a series, as it is really hard to > follow what's going on. There are a couple of things happening: > > - You change types in `struct apply_state`, which bubbles up. > > - You adapt `git_hdr_len()` to receive different inputs, which bubbles > up. > > - You perform small fixes in several places. > > It might also be a good idea to split out the loop counters into a > separate commit, as those are trivially correct.
Sure I'll come up with a v2 patch series, in which each kind of fixes will be put in a single commit and I'll state why I believe the type cast/change is safe for every single fix in the commit message.
Show 11 quoted lines
> > @@ -1087,11 +1086,11 @@ static int gitdiff_index(struct gitdiff_data *state, > > * and optional space with octal mode. > > */ > > const char *ptr, *eol; > > - int len; > > - const unsigned hexsz = the_hash_algo->hexsz; > > + size_t len; > > + const size_t hexsz = the_hash_algo->hexsz; > > The change to `hexsz` shouldn't be needed, even if it makes us match the > type of `hexsz` as declared in `git_hash_algo`.
Yes it's not necessary here to change the type. And for the `hexsz` stuff, on Wed, Feb 05 2025 04:58:57 -0800, Junio C Hamano wrote,
Show 6 quoted lines
> I thought that I already saw this discussed in another thread. > > The .hexsz of any hash algorithm would never be larger than what a > platform natural "unsigned" integer type can hold, so using size_t > for the member _is_ the wrong thing to do and the fix may be the > other way around, no?
I found the discussion mentioned at [1]. It seems like the change here only makes things worse so I'll see if I'd leave it untouched or change the type of `.hexsz` member to `int` or something.
[1] https://lore.kernel.org/git/xmqqttaqw2eb.fsf@gitster.g/
Show 5 quoted lines
> > @@ -2185,7 +2182,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si > > }; > > int i; > > This may arguably be `size_t`, as well.
It's OK for me to use a `size_t` loop count everywhere but I tried to keep the changes in this patch minimal (forget about the `hexsz` thing). I could apply this change if you insist.
Show 17 quoted lines
> > @@ -2257,12 +2255,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)
> > }
> >
> > if (patch->is_binary) {
> > - printf(" %-*s | Bin\n", max, qname.buf);
> > + printf(" %-*s | Bin\n", (int) max, qname.buf);
> > strbuf_release(&qname);
> > return;
> > }
> >
> > - printf(" %-*s |", max, qname.buf);
> > + printf(" %-*s |", (int) max, qname.buf);
> > strbuf_release(&qname);
> >
> > /*
>
> Do we _know_ that `max` fits into an `int`?Yep we've set an upper bound for `max` before:
Show 6 quoted lines
> /* > * "scale" the filename > */ > max = state->max_len; > if (max > 50) > max = 50;
so it must fit into an `int`.