threads / patch / 46801

patch, 4 partscommit-slab.h: avoid -Wsign-compare warnings

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

## tl;dr

4 messages between Sep 21, 2017 and Sep 22, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Ramsay Jones· Sep 21, 2017, 16:47 UTC · lore

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

Show changes to commit-slab.h +3 −3

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

Jeff King· Sep 22, 2017, 05:29 UTC · re: Ramsay Jones · lore

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

On Thu, Sep 21, 2017 at 05:47:36PM +0100, Ramsay Jones wrote:

Show 19 quoted lines

> 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

Ramsay Jones· Sep 22, 2017, 15:46 UTC · re: Jeff King · lore

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

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

Jeff King· Sep 22, 2017, 15:55 UTC · re: Ramsay Jones · lore

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

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

Show 14 quoted lines

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