| Message ID | 20250209081216.241350-2-jelly.zhao.42@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | apply: address -Wsign-comparison warnings | expand |
Zejun Zhao <jelly.zhao.42@gmail.com> writes: > `.max_change` and `.max_len` of `apply_state` are only used as unsigned > integers. Misuse of `int` type would cause -Wsign-comparison warnings. > > Fix this by > > - change `.max_change`'s type to `unsigned` since it's just a counter > Looking at `.max_change` it seems like this is only assigned in `patch_stats()` where we do int lines = patch->lines_added + patch->lines_deleted; if (lines > state->max_change) state->max_change = lines; In this case shouldn't we first convert `.lines_added` `.lines_deleted` to also be 'unsigned int' in the first place? > - change `.max_len`'s type to `size_t` since it's a length > I see that this is assigned the return value of `strlen()` so this makes sense. > - change the types of relevant variables in function `show_stats` > > Note that `printf`'s format string requires us to do some typecast > (from `size_t` to `int`) on `max` in function `show_stats`. This is > safe because we already set a upper bound of `50` for `max` before the > cast. > > Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com> > --- > apply.c | 9 +++++---- > apply.h | 4 ++-- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/apply.c b/apply.c > index 4a7b6120ac..831b338155 100644 > --- a/apply.c > +++ b/apply.c > @@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch) > { > struct strbuf qname = STRBUF_INIT; > char *cp = patch->new_name ? patch->new_name : patch->old_name; > - int max, add, del; > + size_t max; > + unsigned add, del; > Tangential to this patch, I don't think we have a guideline on using 'unsigned' vs 'unsigned int'. Probably we should. > quote_c_style(cp, &qname, NULL, 0); > > @@ -2257,12 +2258,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); > > /* > @@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch) > del = patch->lines_deleted; > > if (state->max_change > 0) { > - int total = ((add + del) * max + state->max_change / 2) / state->max_change; > + unsigned total = ((add + del) * max + state->max_change / 2) / state->max_change; > add = (add * max + state->max_change / 2) / state->max_change; > del = total - add; > } > diff --git a/apply.h b/apply.h > index 90e887ec0e..f7f369d44f 100644 > --- a/apply.h > +++ b/apply.h > @@ -90,8 +90,8 @@ struct apply_state { > * we've seen, and the longest filename. That allows us to do simple > * scaling. > */ > - int max_change; > - int max_len; > + unsigned max_change; > + size_t max_len; > > /* > * Records filenames that have been touched, in order to handle > -- > 2.43.0 The rest look good.
Karthik Nayak <karthik.188@gmail.com> writes: > Zejun Zhao <jelly.zhao.42@gmail.com> writes: > >> `.max_change` and `.max_len` of `apply_state` are only used as unsigned >> integers. Misuse of `int` type would cause -Wsign-comparison warnings. >> >> Fix this by >> >> - change `.max_change`'s type to `unsigned` since it's just a counter >> > > Looking at `.max_change` it seems like this is only assigned in > `patch_stats()` where we do > > int lines = patch->lines_added + patch->lines_deleted; > > if (lines > state->max_change) > state->max_change = lines; > > In this case shouldn't we first convert `.lines_added` `.lines_deleted` > to also be 'unsigned int' in the first place? Surely. Or if any of the internal API uses a calling convention that yields number of lines on success and negative number to signal errors, we could also unify them to signed integer instead. In either case, using types consistently is good, and thanks for sharp eyes spotting this instance. >> @@ -2257,12 +2258,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); >> This is the kind of fallout that makes the resulting code harder to read. How bad would the code churn be if we instead unify to the signed integer type, instead of using size_t, and making sure we use the range-checking versions of arithmetic when needed, I have to wonder?
diff --git a/apply.c b/apply.c index 4a7b6120ac..831b338155 100644 --- a/apply.c +++ b/apply.c @@ -2238,7 +2238,8 @@ static void show_stats(struct apply_state *state, struct patch *patch) { struct strbuf qname = STRBUF_INIT; char *cp = patch->new_name ? patch->new_name : patch->old_name; - int max, add, del; + size_t max; + unsigned add, del; quote_c_style(cp, &qname, NULL, 0); @@ -2257,12 +2258,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); /* @@ -2273,7 +2274,7 @@ static void show_stats(struct apply_state *state, struct patch *patch) del = patch->lines_deleted; if (state->max_change > 0) { - int total = ((add + del) * max + state->max_change / 2) / state->max_change; + unsigned total = ((add + del) * max + state->max_change / 2) / state->max_change; add = (add * max + state->max_change / 2) / state->max_change; del = total - add; } diff --git a/apply.h b/apply.h index 90e887ec0e..f7f369d44f 100644 --- a/apply.h +++ b/apply.h @@ -90,8 +90,8 @@ struct apply_state { * we've seen, and the longest filename. That allows us to do simple * scaling. */ - int max_change; - int max_len; + unsigned max_change; + size_t max_len; /* * Records filenames that have been touched, in order to handle
`.max_change` and `.max_len` of `apply_state` are only used as unsigned integers. Misuse of `int` type would cause -Wsign-comparison warnings. Fix this by - change `.max_change`'s type to `unsigned` since it's just a counter - change `.max_len`'s type to `size_t` since it's a length - change the types of relevant variables in function `show_stats` Note that `printf`'s format string requires us to do some typecast (from `size_t` to `int`) on `max` in function `show_stats`. This is safe because we already set a upper bound of `50` for `max` before the cast. Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com> --- apply.c | 9 +++++---- apply.h | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-)