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

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

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 commit-slab.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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;				\
 									\
 	nth_slab = c->index / s->slab_size;				\
 	nth_slot = c->index % s->slab_size;				\
 									\
 	if (s->slab_count <= nth_slab) {				\
-		int i;							\
+		unsigned int i;						\
 		if (!add_if_missing)					\
 			return NULL;					\
 		REALLOC_ARRAY(s->slab, nth_slab + 1);			\
-- 
2.14.0

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

[Jeff King]

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.

-Peff

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

[Ramsay Jones]

On 22/09/17 06:29, Jeff King wrote:
> 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

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

[Jeff King]

On Fri, Sep 22, 2017 at 04:46:48PM +0100, Ramsay Jones wrote:

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

I _think_ we'd be OK in all of those cases because these size_t's tend
to be once-per-big-list, not once-per-list-item. But I agree that it
would need looking into closely, and definitely should be addressed
separately.

Thanks.

-Peff