From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"GIT Mailing-list" <git@vger.kernel.org>
Subject: Re: [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings
Date: Fri, 22 Sep 2017 18:25:12 +0200 [thread overview]
Message-ID: <20170922162512.7398-1-szeder.dev@gmail.com> (raw)
In-Reply-To: <c97784ce-d85d-2b7a-4eb7-d4043dc1a0b7@ramsayjones.plus.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
next prev parent reply other threads:[~2017-09-22 16:29 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-21 16:49 [PATCH 4/4] ALLOC_GROW: avoid -Wsign-compare warnings Ramsay Jones
2017-09-22 4:20 ` Junio C Hamano
2017-09-22 15:36 ` Ramsay Jones
2017-09-22 16:25 ` SZEDER Gábor [this message]
2017-09-22 19:23 ` Ramsay Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170922162512.7398-1-szeder.dev@gmail.com \
--to=szeder.dev@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=ramsay@ramsayjones.plus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).