Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Sep 22, 2017, 15:46 UTC
Message-ID: <5c82e442-9d3a-d748-248d-3ea55a91f777@ramsayjones.plus.com>
In-Reply-To: <20170922052916.fiafkbykroenq65n@sigill.intra.peff.net>

On 22/09/17 06:29, Jeff King wrote:

Show 26 quoted lines

> On Thu, Sep 21, 2017 at 05:47:36PM +0100, Ramsay Jones wrote:
> 
>> diff --git a/commit-slab.h b/commit-slab.h
>> index 333d81e37..dcaab8ca0 100644
>> --- a/commit-slab.h
>> +++ b/commit-slab.h
>> @@ -78,7 +78,7 @@ static MAYBE_UNUSED void init_ ##slabname(struct slabname *s)		\
>>  									\
>>  static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)		\
>>  {									\
>> -	int i;								\
>> +	unsigned int i;							\
>>  	for (i = 0; i < s->slab_count; i++)				\
>>  		free(s->slab[i]);					\
>>  	s->slab_count = 0;						\
>> @@ -89,13 +89,13 @@ static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,	\
>>  						  const struct commit *c, \
>>  						  int add_if_missing)   \
>>  {									\
>> -	int nth_slab, nth_slot;						\
>> +	unsigned int nth_slab, nth_slot;				\
> 
> I have a feeling that in the long run these should all be size_t, but
> it's probably pretty unlikely to overflow in practice. At any rate, the
> slab index itself is an unsigned, so it probably makes sense to match
> that for now.

Yes, I briefly considered that, but I didn't want to think about possible effects of the increased size of 'struct slabname'. I suspect that it is very unlikely to cause an issue, but I had similar concerns with the 'ALLOC_GROW' patch, were it would have been more likely to cause memory pressure issues (to e.g. s/int/size_t/). I decided that it could be addressed separately, with a patch on top, if necessary.

ATB, Ramsay Jones