| Message ID | 20250209081216.241350-3-jelly.zhao.42@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | apply: address -Wsign-comparison warnings | expand |
On Sun, Feb 09, 2025 at 08:12:12AM +0000, Zejun Zhao wrote: > Some assigned variables are mistyped as `int`, including > > - those whose values come from a system function returning `size_t`, > > - those that are used for array indexing, > > - those that represent length/size/distance, > > some of which will trigger -Wsign-comparison warnings. > > Change some of them to `size_t`/`unsigned`. > > Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com> > --- > apply.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/apply.c b/apply.c > index 831b338155..b4ae74a5fb 100644 > --- a/apply.c > +++ b/apply.c > @@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state, > * and optional space with octal mode. > */ > const char *ptr, *eol; > - int len; > + size_t len; > const unsigned hexsz = the_hash_algo->hexsz; > > ptr = strchr(line, '.'); This is storing the result of `ptr - line`, which will be a positive integer. It's later passed to `memcpy()`, which expects a `size_t`. > @@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage, > { > struct image fixed_preimage = IMAGE_INIT; > size_t insert_pos = 0; > - int i, ctx, reduced; > + int i, reduced; > + size_t ctx; > const char *fixed; > > /* `ctx` is indexing an array and counts against `preimage->line_nr`, which is a `size_t`, too. > @@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state, > struct strbuf fixed = STRBUF_INIT; > char *fixed_buf; > size_t fixed_len; > - int preimage_limit; > + size_t preimage_limit; > int ret; > > if (preimage->line_nr + current_lno <= img->line_nr) { This one stores `struct image::line_nr`, which is a `size_t`. > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state, > { > int i; > unsigned long backwards, forwards, current; > - int backwards_lno, forwards_lno, current_lno; > + size_t backwards_lno, forwards_lno, current_lno; > > /* > * When running with --allow-overlap, it is possible that a hunk is These are a bit curious, as they store `line`, which is itself an `int` parameter. As far as I understand, the only caller is also only ever passing a positive integer here. > @@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state, > */ > static void update_image(struct apply_state *state, > struct image *img, > - int applied_pos, > + size_t applied_pos, > struct image *preimage, > struct image *postimage) > { > @@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state, > size_t remove_count, insert_count, applied_at = 0; > size_t result_alloc; > char *result; > - int preimage_limit; > + size_t preimage_limit; > > /* > * If we are removing blank lines at the end of img, The caller makes sure that the function only gets called when the parameter is a positive integer. > @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch) > > static void patch_stats(struct apply_state *state, struct patch *patch) > { > - int lines = patch->lines_added + patch->lines_deleted; > + unsigned lines = patch->lines_added + patch->lines_deleted; This one is curious again, as the type of these variables is an `int`. This should likely be adapted in tandem if they cannot be negative. Patrick
On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote, > > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state, > > { > > int i; > > unsigned long backwards, forwards, current; > > - int backwards_lno, forwards_lno, current_lno; > > + size_t backwards_lno, forwards_lno, current_lno; > > > > /* > > * When running with --allow-overlap, it is possible that a hunk is > > These are a bit curious, as they store `line`, which is itself an `int` > parameter. As far as I understand, the only caller is also only ever > passing a positive integer here. They do store an `int` parameter, which is `line`. However, we cannot change `line` to unsigned since it can temporarily store an negative value before the assignments to these `*_lno` happen. Below are from function `find_pos` (without any change). /* * If match_beginning or match_end is specified, there is no * point starting from a wrong line that will never match and * wander around and wait for a match at the specified end. */ if (match_beginning) line = 0; else if (match_end) line = img->line_nr - preimage->line_nr; /* * Because the comparison is unsigned, the following test * will also take care of a negative line number that can * result when match_end and preimage is larger than the target. */ if (line > img->line_nr) line = img->line_nr; > > @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch) > > > > static void patch_stats(struct apply_state *state, struct patch *patch) > > { > > - int lines = patch->lines_added + patch->lines_deleted; > > + unsigned lines = patch->lines_added + patch->lines_deleted; > > This one is curious again, as the type of these variables is an `int`. > This should likely be adapted in tandem if they cannot be negative. Thank you for identifying. Will be in next version.
Zejun Zhao <jelly.zhao.42@gmail.com> writes: > On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote, >> > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state, >> > { >> > int i; >> > unsigned long backwards, forwards, current; >> > - int backwards_lno, forwards_lno, current_lno; >> > + size_t backwards_lno, forwards_lno, current_lno; >> > >> > /* >> > * When running with --allow-overlap, it is possible that a hunk is >> >> These are a bit curious, as they store `line`, which is itself an `int` >> parameter. As far as I understand, the only caller is also only ever >> passing a positive integer here. > > They do store an `int` parameter, which is `line`. However, we cannot change > `line` to unsigned since it can temporarily store an negative value before > the assignments to these `*_lno` happen. Correct. Doesn't it mean that a change that makes these line-number variables to size_t is wrong? Of course the change is not made to break the code but may be to please some code paths in other parts of the system that wants these line-number variables that are currently "int" to compare or assign without range-checking with "size_t" quantity or variable, but then shouldn't we fix these places, not the types of these line-number variables that use the platform natural integers? THanks.
diff --git a/apply.c b/apply.c index 831b338155..b4ae74a5fb 100644 --- a/apply.c +++ b/apply.c @@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state, * and optional space with octal mode. */ const char *ptr, *eol; - int len; + size_t len; const unsigned hexsz = the_hash_algo->hexsz; ptr = strchr(line, '.'); @@ -2185,7 +2185,7 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si }; int i; for (i = 0; binhdr[i]; i++) { - int len = strlen(binhdr[i]); + size_t len = strlen(binhdr[i]); if (len < size - hd && !memcmp(binhdr[i], buffer + hd, len)) { state->linenr++; @@ -2320,7 +2320,8 @@ static void update_pre_post_images(struct image *preimage, { struct image fixed_preimage = IMAGE_INIT; size_t insert_pos = 0; - int i, ctx, reduced; + int i, reduced; + size_t ctx; const char *fixed; /* @@ -2492,7 +2493,7 @@ static int match_fragment(struct apply_state *state, struct strbuf fixed = STRBUF_INIT; char *fixed_buf; size_t fixed_len; - int preimage_limit; + size_t preimage_limit; int ret; if (preimage->line_nr + current_lno <= img->line_nr) { @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state, { int i; unsigned long backwards, forwards, current; - int backwards_lno, forwards_lno, current_lno; + size_t backwards_lno, forwards_lno, current_lno; /* * When running with --allow-overlap, it is possible that a hunk is @@ -2791,7 +2792,7 @@ static int find_pos(struct apply_state *state, */ static void update_image(struct apply_state *state, struct image *img, - int applied_pos, + size_t applied_pos, struct image *preimage, struct image *postimage) { @@ -2803,7 +2804,7 @@ static void update_image(struct apply_state *state, size_t remove_count, insert_count, applied_at = 0; size_t result_alloc; char *result; - int preimage_limit; + size_t preimage_limit; /* * If we are removing blank lines at the end of img, @@ -4288,19 +4289,19 @@ static void summary_patch_list(struct patch *patch) static void patch_stats(struct apply_state *state, struct patch *patch) { - int lines = patch->lines_added + patch->lines_deleted; + unsigned lines = patch->lines_added + patch->lines_deleted; if (lines > state->max_change) state->max_change = lines; if (patch->old_name) { - int len = quote_c_style(patch->old_name, NULL, NULL, 0); + size_t len = quote_c_style(patch->old_name, NULL, NULL, 0); if (!len) len = strlen(patch->old_name); if (len > state->max_len) state->max_len = len; } if (patch->new_name) { - int len = quote_c_style(patch->new_name, NULL, NULL, 0); + size_t len = quote_c_style(patch->new_name, NULL, NULL, 0); if (!len) len = strlen(patch->new_name); if (len > state->max_len)
Some assigned variables are mistyped as `int`, including - those whose values come from a system function returning `size_t`, - those that are used for array indexing, - those that represent length/size/distance, some of which will trigger -Wsign-comparison warnings. Change some of them to `size_t`/`unsigned`. Signed-off-by: Zejun Zhao <jelly.zhao.42@gmail.com> --- apply.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)