{"thread":{"id":"46803","subject":"[PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","startedAt":"2017-09-21T16:49:45Z","lastAt":"2017-09-22T19:23:25Z","messageCount":5,"participants":["Ramsay Jones","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"328578","messageId":"c97784ce-d85d-2b7a-4eb7-d4043dc1a0b7@ramsayjones.plus.com","threadId":"46803","inReplyTo":null,"subject":"[PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-21T16:49:38Z","receivedAt":"2017-09-21T16:49:45Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n builtin/pack-objects.c |  4 ++--\n config.c               |  2 +-\n diff.c                 |  2 +-\n line-log.c             | 18 +++++++++---------\n line-log.h             |  2 +-\n revision.c             |  2 +-\n tree-walk.c            |  3 +--\n 7 files changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a57b4f058..a6ee653bf 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2563,8 +2563,8 @@ struct in_pack_object {\n };\n \n struct in_pack {\n-\tint alloc;\n-\tint nr;\n+\tunsigned int alloc;\n+\tunsigned int  nr;\n \tstruct in_pack_object *array;\n };\n \ndiff --git a/config.c b/config.c\nindex cd5a69e63..aeab02c06 100644\n--- a/config.c\n+++ b/config.c\n@@ -2200,7 +2200,7 @@ static struct {\n \tsize_t *offset;\n \tunsigned int offset_alloc;\n \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n-\tint seen;\n+\tunsigned int seen;\n } store;\n \n static int matches(const char *key, const char *value)\ndiff --git a/diff.c b/diff.c\nindex ea7e5978b..be94ad4f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1541,7 +1541,7 @@ static void emit_rewrite_diff(const char *name_a,\n \n struct diff_words_buffer {\n \tmmfile_t text;\n-\tlong alloc;\n+\tunsigned long alloc;\n \tstruct diff_words_orig {\n \t\tconst char *begin, *end;\n \t} *orig;\ndiff --git a/line-log.c b/line-log.c\nindex ab0709f9a..545ad0f28 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -90,7 +90,7 @@ static int range_cmp(const void *_r, const void *_s)\n  */\n static void range_set_check_invariants(struct range_set *rs)\n {\n-\tint i;\n+\tunsigned int i;\n \n \tif (!rs)\n \t\treturn;\n@@ -110,8 +110,8 @@ static void range_set_check_invariants(struct range_set *rs)\n  */\n void sort_and_merge_range_set(struct range_set *rs)\n {\n-\tint i;\n-\tint o = 0; /* output cursor */\n+\tunsigned int i;\n+\tunsigned int o = 0; /* output cursor */\n \n \tQSORT(rs->ranges, rs->nr, range_cmp);\n \n@@ -144,7 +144,7 @@ void sort_and_merge_range_set(struct range_set *rs)\n static void range_set_union(struct range_set *out,\n \t\t\t     struct range_set *a, struct range_set *b)\n {\n-\tint i = 0, j = 0;\n+\tunsigned int i = 0, j = 0;\n \tstruct range *ra = a->ranges;\n \tstruct range *rb = b->ranges;\n \t/* cannot make an alias of out->ranges: it may change during grow */\n@@ -186,7 +186,7 @@ static void range_set_union(struct range_set *out,\n static void range_set_difference(struct range_set *out,\n \t\t\t\t  struct range_set *a, struct range_set *b)\n {\n-\tint i, j =  0;\n+\tunsigned int i, j =  0;\n \tfor (i = 0; i < a->nr; i++) {\n \t\tlong start = a->ranges[i].start;\n \t\tlong end = a->ranges[i].end;\n@@ -397,7 +397,7 @@ static void diff_ranges_filter_touched(struct diff_ranges *out,\n \t\t\t\t       struct diff_ranges *diff,\n \t\t\t\t       struct range_set *rs)\n {\n-\tint i, j = 0;\n+\tunsigned int i, j = 0;\n \n \tassert(out->target.nr == 0);\n \n@@ -426,7 +426,7 @@ static void range_set_shift_diff(struct range_set *out,\n \t\t\t\t struct range_set *rs,\n \t\t\t\t struct diff_ranges *diff)\n {\n-\tint i, j = 0;\n+\tunsigned int i, j = 0;\n \tlong offset = 0;\n \tstruct range *src = rs->ranges;\n \tstruct range *target = diff->target.ranges;\n@@ -873,7 +873,7 @@ static char *output_prefix(struct diff_options *opt)\n \n static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)\n {\n-\tint i, j = 0;\n+\tunsigned int i, j = 0;\n \tlong p_lines, t_lines;\n \tunsigned long *p_ends = NULL, *t_ends = NULL;\n \tstruct diff_filepair *pair = range->pair;\n@@ -906,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \t\tlong t_start = range->ranges.ranges[i].start;\n \t\tlong t_end = range->ranges.ranges[i].end;\n \t\tlong t_cur = t_start;\n-\t\tint j_last;\n+\t\tunsigned int j_last;\n \n \t\twhile (j < diff->target.nr && diff->target.ranges[j].end < t_start)\n \t\t\tj++;\ndiff --git a/line-log.h b/line-log.h\nindex 7a5c24e2d..e2a5ee7c6 100644\n--- a/line-log.h\n+++ b/line-log.h\n@@ -14,7 +14,7 @@ struct range {\n \n /* A set of ranges.  The ranges must always be disjoint and sorted. */\n struct range_set {\n-\tint alloc, nr;\n+\tunsigned int alloc, nr;\n \tstruct range *ranges;\n };\n \ndiff --git a/revision.c b/revision.c\nindex f9a90d71d..c8c9cb32c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1105,7 +1105,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n \t\t\t    unsigned flags)\n {\n \tstruct rev_cmdline_info *info = &revs->cmdline;\n-\tint nr = info->nr;\n+\tunsigned int nr = info->nr;\n \n \tALLOC_GROW(info->rev, nr + 1, info->alloc);\n \tinfo->rev[nr].item = item;\ndiff --git a/tree-walk.c b/tree-walk.c\nindex c99309069..684f0e337 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -582,12 +582,11 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char *tree_s\n \tint retval = MISSING_OBJECT;\n \tstruct dir_state *parents = NULL;\n \tsize_t parents_alloc = 0;\n-\tssize_t parents_nr = 0;\n+\tsize_t i, parents_nr = 0;\n \tunsigned char current_tree_sha1[20];\n \tstruct strbuf namebuf = STRBUF_INIT;\n \tstruct tree_desc t;\n \tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n-\tint i;\n \n \tinit_tree_desc(&t, NULL, 0UL);\n \tstrbuf_addstr(&namebuf, name);\n-- \n2.14.0\n"},{"id":"328614","messageId":"xmqqd16jl5yb.fsf@gitster.mtv.corp.google.com","threadId":"46803","inReplyTo":"c97784ce-d85d-2b7a-4eb7-d4043dc1a0b7@ramsayjones.plus.com","subject":"Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-22T04:20:44Z","receivedAt":"2017-09-22T04:20:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>  builtin/pack-objects.c |  4 ++--\n>  config.c               |  2 +-\n>  diff.c                 |  2 +-\n>  line-log.c             | 18 +++++++++---------\n>  line-log.h             |  2 +-\n>  revision.c             |  2 +-\n>  tree-walk.c            |  3 +--\n>  7 files changed, 16 insertions(+), 17 deletions(-)\n\nThanks.  \n\nI did not spot any questionable conversion (e.g. \"something that\nused to be signed was because it wanted to store -1 as a sentinel\"\nwould be broken if we just change that to unsigned) by going over\nthe places these variables and fields are actually used.\n\nA review of a patch like this involves reading through 10x more\nlines than we see in the above diffstat, and producing it would most\nlikely have taken the same amount of effort, at least.  Very much\nappreciated.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index a57b4f058..a6ee653bf 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2563,8 +2563,8 @@ struct in_pack_object {\n>  };\n>  \n>  struct in_pack {\n> -\tint alloc;\n> -\tint nr;\n> +\tunsigned int alloc;\n> +\tunsigned int  nr;\n\nThis is a bit questionable ;-) but it is something I can locally\ntweak easily.\n\n"},{"id":"328637","messageId":"b3846c1f-0335-8afa-c155-c2726f57679c@ramsayjones.plus.com","threadId":"46803","inReplyTo":"xmqqd16jl5yb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-22T15:36:24Z","receivedAt":"2017-09-22T15:36:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/09/17 05:20, Junio C Hamano wrote:\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index a57b4f058..a6ee653bf 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -2563,8 +2563,8 @@ struct in_pack_object {\n>>  };\n>>  \n>>  struct in_pack {\n>> -\tint alloc;\n>> -\tint nr;\n>> +\tunsigned int alloc;\n>> +\tunsigned int  nr;\n> \n> This is a bit questionable ;-) but it is something I can locally\n> tweak easily.\n\nHeh, it took me a minute to see what you were referring to! ;-)\nYep, sorry, I don't know how I managed to add the extra space.\n(I don't think I can blame vim - PEBKAC).\n\nATB,\nRamsay Jones\n\n\n"},{"id":"328646","messageId":"20170922162512.7398-1-szeder.dev@gmail.com","threadId":"46803","inReplyTo":"c97784ce-d85d-2b7a-4eb7-d4043dc1a0b7@ramsayjones.plus.com","subject":"Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-09-22T16:25:12Z","receivedAt":"2017-09-22T16:29:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nAt first I was somewhat puzzled by the \"ALLOC_GROW:\" prefix in the\nsubject line, because this patch doesn't touch ALLOC_GROW() at all.\nHowever, since ALLOC_GROW() is a macro, of course, and since this\npatch changes the data type of variables \"passed\" to ALLOC_GROW(),\nthat's sort of fine...\n\nBut then I was even more puzzled to see that this patch also changes\nthe data type of several variables that are never passed to\nALLOC_GROW(), but only compared to other variables that are indeed\npassed to ALLOC_GROW(), i.e. most of (all?) the changes in line-log.c.\nPerhaps it would be worth mentioning that all those changes are\nfallout of the type change in 'struct range_set' in line-log.h. (and\nall those changes silence only two warnings!)\n\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>  builtin/pack-objects.c |  4 ++--\n>  config.c               |  2 +-\n>  diff.c                 |  2 +-\n>  line-log.c             | 18 +++++++++---------\n>  line-log.h             |  2 +-\n>  revision.c             |  2 +-\n>  tree-walk.c            |  3 +--\n>  7 files changed, 16 insertions(+), 17 deletions(-)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index a57b4f058..a6ee653bf 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2563,8 +2563,8 @@ struct in_pack_object {\n>  };\n>  \n>  struct in_pack {\n> -\tint alloc;\n> -\tint nr;\n> +\tunsigned int alloc;\n> +\tunsigned int  nr;\n>  \tstruct in_pack_object *array;\n>  };\n>  \n> diff --git a/config.c b/config.c\n> index cd5a69e63..aeab02c06 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2200,7 +2200,7 @@ static struct {\n>  \tsize_t *offset;\n>  \tunsigned int offset_alloc;\n>  \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n> -\tint seen;\n> +\tunsigned int seen;\n>  } store;\n\nOn first sight this looked like an independent change, but on closer\ninspection it turns out that the variables 'seen' and 'offset_alloc'\nare used to manage the allocation of the '*offset' array.\n\nI wish we would have named these fields more consistently with '_nr'\nand '_alloc' suffixes, or, if there is a compelling reason to diverge,\nthen at least put the two fields on subsequent lines (or even on the\nsame line), with a comment explaining the connection between the two\nfields and the array.\n\n>  static int matches(const char *key, const char *value)\n> diff --git a/diff.c b/diff.c\n> index ea7e5978b..be94ad4f4 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1541,7 +1541,7 @@ static void emit_rewrite_diff(const char *name_a,\n>  \n>  struct diff_words_buffer {\n>  \tmmfile_t text;\n> -\tlong alloc;\n> +\tunsigned long alloc;\n\nThis one is interesting.  'alloc' and 'mmfile_t's 'text.size' manage\nthe allocation of 'text.ptr', and both are signed longs...  so where\ndoes the warning come from?  Well, just a couple of lines later we\nhave this:\n\n  static void diff_words_append(char *line, unsigned long len,\n                  struct diff_words_buffer *buffer)\n  {\n          ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);\n\nNote the addition of the signed long 'buffer->text.size' and the\nunsigned long 'len', which, according to \"6.3.1.8 Usual arithmetic\nconversions\", converts the signed long to unsigned.  ALLOC_GROW() then\ncompares the resulting unsigned long sum to the signed long\n'buffer->alloc', hence the warning.\n\nSo, while the change in this hunk is technically correct and indeed\neliminates the warning, it is subtle and the resulting code with a\nsigned long 'text.size' in 'mmfile_t' and unsigned long 'alloc' might\nraise the eyebrows of future readers.  I think this would be worth\nmentioning in the commit message or in a comment.\n\nUltimately 'text.size' should be turned into unsigned, too, maybe even\nsize_t, but that change would be much more difficult to make and\nreview, because mmfile_t is used over hundred times in our codebase,\nand 'size' is not a grep-friendly field name to look for.\n\n>  \tstruct diff_words_orig {\n>  \t\tconst char *begin, *end;\n>  \t} *orig;\n\nThe very next line of 'struct diff_words_buffer's definition is:\n\n    int orig_nr, orig_alloc;\n\nThese two fields are used to manage the allocation of the struct's\n'*orig' array.  While these are not involved in any warnings, having\nan 'unsigned long alloc' and a signed 'orig_alloc' so close to each\nother in the same struct might raise some eyebrows, too.\n\n> diff --git a/line-log.c b/line-log.c\n> index ab0709f9a..545ad0f28 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -90,7 +90,7 @@ static int range_cmp(const void *_r, const void *_s)\n>   */\n>  static void range_set_check_invariants(struct range_set *rs)\n>  {\n> -\tint i;\n> +\tunsigned int i;\n>  \n>  \tif (!rs)\n>  \t\treturn;\n> @@ -110,8 +110,8 @@ static void range_set_check_invariants(struct range_set *rs)\n>   */\n>  void sort_and_merge_range_set(struct range_set *rs)\n>  {\n> -\tint i;\n> -\tint o = 0; /* output cursor */\n> +\tunsigned int i;\n> +\tunsigned int o = 0; /* output cursor */\n>  \n>  \tQSORT(rs->ranges, rs->nr, range_cmp);\n>  \n> @@ -144,7 +144,7 @@ void sort_and_merge_range_set(struct range_set *rs)\n>  static void range_set_union(struct range_set *out,\n>  \t\t\t     struct range_set *a, struct range_set *b)\n>  {\n> -\tint i = 0, j = 0;\n> +\tunsigned int i = 0, j = 0;\n>  \tstruct range *ra = a->ranges;\n>  \tstruct range *rb = b->ranges;\n>  \t/* cannot make an alias of out->ranges: it may change during grow */\n> @@ -186,7 +186,7 @@ static void range_set_union(struct range_set *out,\n>  static void range_set_difference(struct range_set *out,\n>  \t\t\t\t  struct range_set *a, struct range_set *b)\n>  {\n> -\tint i, j =  0;\n> +\tunsigned int i, j =  0;\n>  \tfor (i = 0; i < a->nr; i++) {\n>  \t\tlong start = a->ranges[i].start;\n>  \t\tlong end = a->ranges[i].end;\n> @@ -397,7 +397,7 @@ static void diff_ranges_filter_touched(struct diff_ranges *out,\n>  \t\t\t\t       struct diff_ranges *diff,\n>  \t\t\t\t       struct range_set *rs)\n>  {\n> -\tint i, j = 0;\n> +\tunsigned int i, j = 0;\n>  \n>  \tassert(out->target.nr == 0);\n>  \n> @@ -426,7 +426,7 @@ static void range_set_shift_diff(struct range_set *out,\n>  \t\t\t\t struct range_set *rs,\n>  \t\t\t\t struct diff_ranges *diff)\n>  {\n> -\tint i, j = 0;\n> +\tunsigned int i, j = 0;\n>  \tlong offset = 0;\n>  \tstruct range *src = rs->ranges;\n>  \tstruct range *target = diff->target.ranges;\n> @@ -873,7 +873,7 @@ static char *output_prefix(struct diff_options *opt)\n>  \n>  static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)\n>  {\n> -\tint i, j = 0;\n> +\tunsigned int i, j = 0;\n>  \tlong p_lines, t_lines;\n>  \tunsigned long *p_ends = NULL, *t_ends = NULL;\n>  \tstruct diff_filepair *pair = range->pair;\n> @@ -906,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n>  \t\tlong t_start = range->ranges.ranges[i].start;\n>  \t\tlong t_end = range->ranges.ranges[i].end;\n>  \t\tlong t_cur = t_start;\n> -\t\tint j_last;\n> +\t\tunsigned int j_last;\n>  \n>  \t\twhile (j < diff->target.nr && diff->target.ranges[j].end < t_start)\n>  \t\t\tj++;\n> diff --git a/line-log.h b/line-log.h\n> index 7a5c24e2d..e2a5ee7c6 100644\n> --- a/line-log.h\n> +++ b/line-log.h\n> @@ -14,7 +14,7 @@ struct range {\n>  \n>  /* A set of ranges.  The ranges must always be disjoint and sorted. */\n>  struct range_set {\n> -\tint alloc, nr;\n> +\tunsigned int alloc, nr;\n>  \tstruct range *ranges;\n>  };\n>  \n> diff --git a/revision.c b/revision.c\n> index f9a90d71d..c8c9cb32c 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1105,7 +1105,7 @@ static void add_rev_cmdline(struct rev_info *revs,\n>  \t\t\t    unsigned flags)\n>  {\n>  \tstruct rev_cmdline_info *info = &revs->cmdline;\n> -\tint nr = info->nr;\n> +\tunsigned int nr = info->nr;\n>  \n>  \tALLOC_GROW(info->rev, nr + 1, info->alloc);\n>  \tinfo->rev[nr].item = item;\n> diff --git a/tree-walk.c b/tree-walk.c\n> index c99309069..684f0e337 100644\n> --- a/tree-walk.c\n> +++ b/tree-walk.c\n> @@ -582,12 +582,11 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char *tree_s\n>  \tint retval = MISSING_OBJECT;\n>  \tstruct dir_state *parents = NULL;\n>  \tsize_t parents_alloc = 0;\n> -\tssize_t parents_nr = 0;\n> +\tsize_t i, parents_nr = 0;\n>  \tunsigned char current_tree_sha1[20];\n>  \tstruct strbuf namebuf = STRBUF_INIT;\n>  \tstruct tree_desc t;\n>  \tint follows_remaining = GET_TREE_ENTRY_FOLLOW_SYMLINKS_MAX_LINKS;\n> -\tint i;\n>  \n>  \tinit_tree_desc(&t, NULL, 0UL);\n>  \tstrbuf_addstr(&namebuf, name);\n> -- \n> 2.14.0\n\n"},{"id":"328663","messageId":"49da282f-3a9d-4d0a-c21f-a96e5eb4410f@ramsayjones.plus.com","threadId":"46803","inReplyTo":"20170922162512.7398-1-szeder.dev@gmail.com","subject":"Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-22T19:23:17Z","receivedAt":"2017-09-22T19:23:25Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/09/17 17:25, SZEDER Gábor wrote:\n> \n> At first I was somewhat puzzled by the \"ALLOC_GROW:\" prefix in the\n> subject line, because this patch doesn't touch ALLOC_GROW() at all.\n> However, since ALLOC_GROW() is a macro, of course, and since this\n> patch changes the data type of variables \"passed\" to ALLOC_GROW(),\n> that's sort of fine...\n\nYes, the original subject line was \"... when using the ALLOC_GROW macro\",\nbut vim scolded me for busting the line length. I tried several other\nvariations, but I couldn't come up with anything better.\n\nSo, yes, given that the subject left a little to be desired, I probably\nshould have included a commit message body. :(\n\n[This patch was originally written years ago, as part of a much larger\nseries to fix all -Wextra warnings. I was pleasantly surprised that it\napplied to master without conflicts. However, I had to add to the patch\nbecause new instances of -Wsign-compare due to using the ALLOC_GROW macro\nhad appeared since then.]\n\n> But then I was even more puzzled to see that this patch also changes\n> the data type of several variables that are never passed to\n> ALLOC_GROW(), but only compared to other variables that are indeed\n> passed to ALLOC_GROW(), i.e. most of (all?) the changes in line-log.c.\n> Perhaps it would be worth mentioning that all those changes are\n> fallout of the type change in 'struct range_set' in line-log.h. (and\n> all those changes silence only two warnings!)\n\nHmm, I did consider splitting this patch up, so that this (and other\nissues you mention below) could be called out separately, but well ... ;-)\n\n>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n>> ---\n>>  builtin/pack-objects.c |  4 ++--\n>>  config.c               |  2 +-\n>>  diff.c                 |  2 +-\n>>  line-log.c             | 18 +++++++++---------\n>>  line-log.h             |  2 +-\n>>  revision.c             |  2 +-\n>>  tree-walk.c            |  3 +--\n>>  7 files changed, 16 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n>> index a57b4f058..a6ee653bf 100644\n>> --- a/builtin/pack-objects.c\n>> +++ b/builtin/pack-objects.c\n>> @@ -2563,8 +2563,8 @@ struct in_pack_object {\n>>  };\n>>  \n>>  struct in_pack {\n>> -\tint alloc;\n>> -\tint nr;\n>> +\tunsigned int alloc;\n>> +\tunsigned int  nr;\n>>  \tstruct in_pack_object *array;\n>>  };\n>>  \n>> diff --git a/config.c b/config.c\n>> index cd5a69e63..aeab02c06 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -2200,7 +2200,7 @@ static struct {\n>>  \tsize_t *offset;\n>>  \tunsigned int offset_alloc;\n>>  \tenum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;\n>> -\tint seen;\n>> +\tunsigned int seen;\n>>  } store;\n> \n> On first sight this looked like an independent change, but on closer\n> inspection it turns out that the variables 'seen' and 'offset_alloc'\n> are used to manage the allocation of the '*offset' array.\n> \n> I wish we would have named these fields more consistently with '_nr'\n> and '_alloc' suffixes, or, if there is a compelling reason to diverge,\n> then at least put the two fields on subsequent lines (or even on the\n> same line), with a comment explaining the connection between the two\n> fields and the array.\n\nYes, I agree. If I had split this patch up, I would have considered\nadding such modifications to that patch. (That's easy to say now, of\ncourse!)\n\n>>  static int matches(const char *key, const char *value)\n>> diff --git a/diff.c b/diff.c\n>> index ea7e5978b..be94ad4f4 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1541,7 +1541,7 @@ static void emit_rewrite_diff(const char *name_a,\n>>  \n>>  struct diff_words_buffer {\n>>  \tmmfile_t text;\n>> -\tlong alloc;\n>> +\tunsigned long alloc;\n> \n> This one is interesting.  'alloc' and 'mmfile_t's 'text.size' manage\n> the allocation of 'text.ptr', and both are signed longs...  so where\n> does the warning come from?  Well, just a couple of lines later we\n> have this:\n> \n>   static void diff_words_append(char *line, unsigned long len,\n>                   struct diff_words_buffer *buffer)\n>   {\n>           ALLOC_GROW(buffer->text.ptr, buffer->text.size + len, buffer->alloc);\n> \n> Note the addition of the signed long 'buffer->text.size' and the\n> unsigned long 'len', which, according to \"6.3.1.8 Usual arithmetic\n> conversions\", converts the signed long to unsigned.  ALLOC_GROW() then\n> compares the resulting unsigned long sum to the signed long\n> 'buffer->alloc', hence the warning.\n> \n> So, while the change in this hunk is technically correct and indeed\n> eliminates the warning, it is subtle and the resulting code with a\n> signed long 'text.size' in 'mmfile_t' and unsigned long 'alloc' might\n> raise the eyebrows of future readers.  I think this would be worth\n> mentioning in the commit message or in a comment.\n> \n> Ultimately 'text.size' should be turned into unsigned, too, maybe even\n> size_t, but that change would be much more difficult to make and\n> review, because mmfile_t is used over hundred times in our codebase,\n> and 'size' is not a grep-friendly field name to look for.\n\nIndeed, ... :-P\n\n>>  \tstruct diff_words_orig {\n>>  \t\tconst char *begin, *end;\n>>  \t} *orig;\n> \n> The very next line of 'struct diff_words_buffer's definition is:\n> \n>     int orig_nr, orig_alloc;\n> \n> These two fields are used to manage the allocation of the struct's\n> '*orig' array.  While these are not involved in any warnings, having\n> an 'unsigned long alloc' and a signed 'orig_alloc' so close to each\n> other in the same struct might raise some eyebrows, too.\nThanks for the detailed review.\n\nATB,\nRamsay Jones\n\n\n"}]}