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

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

[Ramsay Jones]

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;
 
 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;
 	struct diff_words_orig {
 		const char *begin, *end;
 	} *orig;
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

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> 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(-)

Thanks.  

I did not spot any questionable conversion (e.g. "something that
used to be signed was because it wanted to store -1 as a sentinel"
would be broken if we just change that to unsigned) by going over
the places these variables and fields are actually used.

A review of a patch like this involves reading through 10x more
lines than we see in the above diffstat, and producing it would most
likely have taken the same amount of effort, at least.  Very much
appreciated.

> 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;

This is a bit questionable ;-) but it is something I can locally
tweak easily.

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

[Ramsay Jones]

On 22/09/17 05:20, Junio C Hamano wrote:
>> 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;
> 
> This is a bit questionable ;-) but it is something I can locally
> tweak easily.

Heh, it took me a minute to see what you were referring to! ;-)
Yep, sorry, I don't know how I managed to add the extra space.
(I don't think I can blame vim - PEBKAC).

ATB,
Ramsay Jones

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

[SZEDER Gábor]

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

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

[Ramsay Jones]

On 22/09/17 17:25, SZEDER Gábor wrote:
> 
> 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...

Yes, the original subject line was "... when using the ALLOC_GROW macro",
but vim scolded me for busting the line length. I tried several other
variations, but I couldn't come up with anything better.

So, yes, given that the subject left a little to be desired, I probably
should have included a commit message body. :(

[This patch was originally written years ago, as part of a much larger
series to fix all -Wextra warnings. I was pleasantly surprised that it
applied to master without conflicts. However, I had to add to the patch
because new instances of -Wsign-compare due to using the ALLOC_GROW macro
had appeared since then.]

> 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!)

Hmm, I did consider splitting this patch up, so that this (and other
issues you mention below) could be called out separately, but well ... ;-)

>> 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.

Yes, I agree. If I had split this patch up, I would have considered
adding such modifications to that patch. (That's easy to say now, of
course!)

>>  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.

Indeed, ... :-P

>>  	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.
Thanks for the detailed review.

ATB,
Ramsay Jones