At first I was somewhat puzzled by the "ALLOC_GROW:" prefix in the
subject line, because this patch doesn't touch ALLOC_GROW() at all.
However, since ALLOC_GROW() is a macro, of course, and since this
patch changes the data type of variables "passed" to ALLOC_GROW(),
that's sort of fine...
But then I was even more puzzled to see that this patch also changes
the data type of several variables that are never passed to
ALLOC_GROW(), but only compared to other variables that are indeed
passed to ALLOC_GROW(), i.e. most of (all?) the changes in line-log.c.
Perhaps it would be worth mentioning that all those changes are
fallout of the type change in 'struct range_set' in line-log.h. (and
all those changes silence only two warnings!)
> Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
> ---
> builtin/pack-objects.c | 4 ++--
> config.c | 2 +-
> diff.c | 2 +-
> line-log.c | 18 +++++++++---------
> line-log.h | 2 +-
> revision.c | 2 +-
> tree-walk.c | 3 +--
> 7 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a57b4f058..a6ee653bf 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2563,8 +2563,8 @@ struct in_pack_object {
> };
>
> struct in_pack {
> - int alloc;
> - int nr;
> + unsigned int alloc;
> + unsigned int nr;
> struct in_pack_object *array;
> };
>
> diff --git a/config.c b/config.c
> index cd5a69e63..aeab02c06 100644
> --- a/config.c
> +++ b/config.c
> @@ -2200,7 +2200,7 @@ static struct {
> size_t *offset;
> unsigned int offset_alloc;
> enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;
> - int seen;
> + unsigned int seen;
> } store;
On first sight this looked like an independent change, but on closer
inspection it turns out that the variables 'seen' and 'offset_alloc'
are used to manage the allocation of the '*offset' array.
I wish we would have named these fields more consistently with '_nr'
and '_alloc' suffixes, or, if there is a compelling reason to diverge,
then at least put the two fields on subsequent lines (or even on the
same line), with a comment explaining the connection between the two
fields and the array.
> static int matches(const char *key, const char *value)
> diff --git a/diff.c b/diff.c
> index ea7e5978b..be94ad4f4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1541,7 +1541,7 @@ static void emit_rewrite_diff(const char *name_a,
>
> struct diff_words_buffer {
> mmfile_t text;
> - long alloc;
> + unsigned long alloc;
This one is interesting. 'alloc' and 'mmfile_t's 'text.size' manage
the allocation of 'text.ptr', and both are signed longs... so where
does the warning come from? Well, just a couple of lines later we
have this:
static void diff_words_append(char *line, unsigned long len,
struct diff_words_buffer *buffer)
{
ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);
Note the addition of the signed long 'buffer->text.size' and the
unsigned long 'len', which, according to "6.3.1.8 Usual arithmetic
conversions", converts the signed long to unsigned. ALLOC_GROW() then
compares the resulting unsigned long sum to the signed long
'buffer->alloc', hence the warning.
So, while the change in this hunk is technically correct and indeed
eliminates the warning, it is subtle and the resulting code with a
signed long 'text.size' in 'mmfile_t' and unsigned long 'alloc' might
raise the eyebrows of future readers. I think this would be worth
mentioning in the commit message or in a comment.
Ultimately 'text.size' should be turned into unsigned, too, maybe even
size_t, but that change would be much more difficult to make and
review, because mmfile_t is used over hundred times in our codebase,
and 'size' is not a grep-friendly field name to look for.
> struct diff_words_orig {
> const char *begin, *end;
> } *orig;
The very next line of 'struct diff_words_buffer's definition is:
int orig_nr, orig_alloc;
These two fields are used to manage the allocation of the struct's
'*orig' array. While these are not involved in any warnings, having
an 'unsigned long alloc' and a signed 'orig_alloc' so close to each
other in the same struct might raise some eyebrows, too.
> diff --git a/line-log.c b/line-log.c
> index ab0709f9a..545ad0f28 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -90,7 +90,7 @@ static int range_cmp(const void *_r, const void *_s)
> */
> static void range_set_check_invariants(struct range_set *rs)
> {
> - int i;
> + unsigned int i;
>
> if (!rs)
> return;
> @@ -110,8 +110,8 @@ static void range_set_check_invariants(struct range_set *rs)
> */
> void sort_and_merge_range_set(struct range_set *rs)
> {
> - int i;
> - int o = 0; /* output cursor */
> + unsigned int i;
> + unsigned int o = 0; /* output cursor */
>
> QSORT(rs->ranges, rs->nr, range_cmp);
>
> @@ -144,7 +144,7 @@ void sort_and_merge_range_set(struct range_set *rs)
> static void range_set_union(struct range_set *out,
> struct range_set *a, struct range_set *b)
> {
> - int i = 0, j = 0;
> + unsigned int i = 0, j = 0;
> struct range *ra = a->ranges;
> struct range *rb = b->ranges;
> /* cannot make an alias of out->ranges: it may change during grow */
> @@ -186,7 +186,7 @@ static void range_set_union(struct range_set *out,
> static void range_set_difference(struct range_set *out,
> struct range_set *a, struct range_set *b)
> {
> - int i, j = 0;
> + unsigned int i, j = 0;
> for (i = 0; i < a->nr; i++) {
> long start = a->ranges[i].start;
> long end = a->ranges[i].end;
> @@ -397,7 +397,7 @@ static void diff_ranges_filter_touched(struct diff_ranges *out,
> struct diff_ranges *diff,
> struct range_set *rs)
> {
> - int i, j = 0;
> + unsigned int i, j = 0;
>
> assert(out->target.nr == 0);
>
> @@ -426,7 +426,7 @@ static void range_set_shift_diff(struct range_set *out,
> struct range_set *rs,
> struct diff_ranges *diff)
> {
> - int i, j = 0;
> + unsigned int i, j = 0;
> long offset = 0;
> struct range *src = rs->ranges;
> struct range *target = diff->target.ranges;
> @@ -873,7 +873,7 @@ static char *output_prefix(struct diff_options *opt)
>
> static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)
> {
> - int i, j = 0;
> + unsigned int i, j = 0;
> long p_lines, t_lines;
> unsigned long *p_ends = NULL, *t_ends = NULL;
> struct diff_filepair *pair = range->pair;
> @@ -906,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
> long t_start = range->ranges.ranges[i].start;
> long t_end = range->ranges.ranges[i].end;
> long t_cur = t_start;
> - int j_last;
> + unsigned int j_last;
>
> while (j < diff->target.nr && diff->target.ranges[j].end < t_start)
> j++;
> diff --git a/line-log.h b/line-log.h
> index 7a5c24e2d..e2a5ee7c6 100644
> --- a/line-log.h
> +++ b/line-log.h
> @@ -14,7 +14,7 @@ struct range {
>
> /* A set of ranges. The ranges must always be disjoint and sorted. */
> struct range_set {
> - int alloc, nr;
> + unsigned int alloc, nr;
> struct range *ranges;
> };
>
> diff --git a/revision.c b/revision.c
> index f9a90d71d..c8c9cb32c 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1105,7 +1105,7 @@ static void add_rev_cmdline(struct rev_info *revs,
> unsigned flags)
> {
> struct rev_cmdline_info *info = &revs->cmdline;
> - int nr = info->nr;
> + unsigned int nr = info->nr;
>
> ALLOC_GROW(info->rev, nr + 1, info->alloc);
> info->rev[nr].item = item;
> diff --git a/tree-walk.c b/tree-walk.c
> index c99309069..684f0e337 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -582,12 +582,11 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char *tree_s
> int retval = MISSING_OBJECT;
> struct dir_state *parents = NULL;
> size_t parents_alloc = 0;
> - ssize_t parents_nr = 0;
> + size_t i, parents_nr = 0;
> unsigned char current_tree_sha1[20];
> struct strbuf namebuf = STRBUF_INIT;
> struct tree_desc t;
> int follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;
> - int i;
>
> init_tree_desc(&t, NULL, 0UL);
> strbuf_addstr(&namebuf, name);
> --
> 2.14.0
