Re: [PATCH 10/10] reftable: address trivial -Wsign-compare warnings

[Junio C Hamano <gitster@pobox.com>]

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.

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

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