Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings

["SZEDER Gábor" <szeder.dev@gmail.com>]

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@ramsayjones.plus.com>
> ---
>  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