# [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings 4 messages from 2017-09-21 to 2017-09-22. Participants: Ramsay Jones, Jeff King. Thread: https://gitlist.dev/t/46801 ## Ramsay Jones, 2017-09-21 16:47 Subject: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings Message-ID: URL: https://gitlist.dev/e/f120046a-f0b3-6199-a2fc-65a5fd37c0a3%40ramsayjones.plus.com ``` Signed-off-by: Ramsay Jones --- 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 ``` ## Jeff King, 2017-09-22 05:29 Subject: Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings Message-ID: <20170922052916.fiafkbykroenq65n@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170922052916.fiafkbykroenq65n%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Ramsay Jones, 2017-09-22 15:46 Subject: Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings Message-ID: <5c82e442-9d3a-d748-248d-3ea55a91f777@ramsayjones.plus.com> URL: https://gitlist.dev/e/5c82e442-9d3a-d748-248d-3ea55a91f777%40ramsayjones.plus.com In-Reply-To: <20170922052916.fiafkbykroenq65n@sigill.intra.peff.net> ``` 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 ``` ## Jeff King, 2017-09-22 15:55 Subject: Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings Message-ID: <20170922155516.wcvpp37usb34ed3i@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170922155516.wcvpp37usb34ed3i%40sigill.intra.peff.net In-Reply-To: <5c82e442-9d3a-d748-248d-3ea55a91f777@ramsayjones.plus.com> ``` 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 ```