Re: [PATCH 10/10] reftable: address trivial -Wsign-compare warnings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 16, 2025, 22:12 UTC
- Message-ID
- <xmqqldvapfvy.fsf@gitster.g>
- In-Reply-To
- <20250116-b4-pks-reftable-sign-compare-v1-10-bd30e2ee96e7@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Address the last couple of trivial -Wsign-compare warnings in the > reftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that > we have in "reftable/system.h".
Hmph.
> - int l = strlen(str); > + size_t l = strlen(str);
Obviously correct.
> - j = 1;
> - while (j < count) {
> + for (uint64_t j = 1; j < count; j++) {OK. As count is u64, it makes sense to match.
Show 13 quoted lines
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 531660a49f..5c0d6273a7 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st)
> }
>
> if (st->readers) {
> - int i = 0;
> struct reftable_buf filename = REFTABLE_BUF_INIT;
> - for (i = 0; i < st->readers_len; i++) {
> +
> + for (size_t i = 0; i < st->readers_len; i++) {Likewise, readers_len is size_t so counters can match it.
> - for (i = 0; i < st->readers_len; i++) {
> + for (size_t i = 0; i < st->readers_len; i++) {Ditto.
> - for (i = 0; !found && i < st->readers_len; i++) {
> + for (size_t i = 0; !found && i < st->readers_len; i++)Ditto.
Show 11 quoted lines
> diff --git a/reftable/writer.c b/reftable/writer.c
> index 4e6ca2e368..91d6629486 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w)
>
> struct common_prefix_arg {
> struct reftable_buf *last;
> - int max;
> + size_t max;
> };This is dubious. write.c:update_common() uses this to keep track of the maximum value returned by common_prefix_size(), which returns an int. writer.c:writer_dump_object_index() assigns the value comparable to this member to reftable_stats.object_id_len that is of type int.
I may be more sympathetic if we were making common_prefix_size() return size_t instead of "int" and dealing with all the fallouts from such a change, but this smells more like somebody _else_ is using size_t on something that is not an allocation size, and such mixed use is cascading down to contaminate this member, which would be perfectly fine with platform native "int".
Ah, OK, an earlier patch does change these other things to size_t, so this must change to size_t to be consistent. Shouldn't it be done in the same patch, then, though?