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

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

quoted

Signed-off-by: Ramsay Jones <redacted>
---
 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!)

quoted

 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

quoted

 	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