{"thread":{"id":"46801","subject":"[PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings","startedAt":"2017-09-21T16:47:45Z","lastAt":"2017-09-22T15:55:23Z","messageCount":4,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"328576","messageId":"f120046a-f0b3-6199-a2fc-65a5fd37c0a3@ramsayjones.plus.com","threadId":"46801","inReplyTo":null,"subject":"[PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-21T16:47:36Z","receivedAt":"2017-09-21T16:47:45Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n commit-slab.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex 333d81e37..dcaab8ca0 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -78,7 +78,7 @@ static MAYBE_UNUSED void init_ ##slabname(struct slabname *s)\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n {\t\t\t\t\t\t\t\t\t\\\n-\tint i;\t\t\t\t\t\t\t\t\\\n+\tunsigned int i;\t\t\t\t\t\t\t\\\n \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n \t\tfree(s->slab[i]);\t\t\t\t\t\\\n \ts->slab_count = 0;\t\t\t\t\t\t\\\n@@ -89,13 +89,13 @@ static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n \t\t\t\t\t\t  const struct commit *c, \\\n \t\t\t\t\t\t  int add_if_missing)   \\\n {\t\t\t\t\t\t\t\t\t\\\n-\tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n+\tunsigned int nth_slab, nth_slot;\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n \tnth_slab = c->index / s->slab_size;\t\t\t\t\\\n \tnth_slot = c->index % s->slab_size;\t\t\t\t\\\n \t\t\t\t\t\t\t\t\t\\\n \tif (s->slab_count <= nth_slab) {\t\t\t\t\\\n-\t\tint i;\t\t\t\t\t\t\t\\\n+\t\tunsigned int i;\t\t\t\t\t\t\\\n \t\tif (!add_if_missing)\t\t\t\t\t\\\n \t\t\treturn NULL;\t\t\t\t\t\\\n \t\tREALLOC_ARRAY(s->slab, nth_slab + 1);\t\t\t\\\n-- \n2.14.0\n"},{"id":"328622","messageId":"20170922052916.fiafkbykroenq65n@sigill.intra.peff.net","threadId":"46801","inReplyTo":"f120046a-f0b3-6199-a2fc-65a5fd37c0a3@ramsayjones.plus.com","subject":"Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T05:29:16Z","receivedAt":"2017-09-22T05:29:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2017 at 05:47:36PM +0100, Ramsay Jones wrote:\n\n> diff --git a/commit-slab.h b/commit-slab.h\n> index 333d81e37..dcaab8ca0 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -78,7 +78,7 @@ static MAYBE_UNUSED void init_ ##slabname(struct slabname *s)\t\t\\\n>  \t\t\t\t\t\t\t\t\t\\\n>  static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n>  {\t\t\t\t\t\t\t\t\t\\\n> -\tint i;\t\t\t\t\t\t\t\t\\\n> +\tunsigned int i;\t\t\t\t\t\t\t\\\n>  \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n>  \t\tfree(s->slab[i]);\t\t\t\t\t\\\n>  \ts->slab_count = 0;\t\t\t\t\t\t\\\n> @@ -89,13 +89,13 @@ static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n>  \t\t\t\t\t\t  const struct commit *c, \\\n>  \t\t\t\t\t\t  int add_if_missing)   \\\n>  {\t\t\t\t\t\t\t\t\t\\\n> -\tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n> +\tunsigned int nth_slab, nth_slot;\t\t\t\t\\\n\nI have a feeling that in the long run these should all be size_t, but\nit's probably pretty unlikely to overflow in practice. At any rate, the\nslab index itself is an unsigned, so it probably makes sense to match\nthat for now.\n\n-Peff\n"},{"id":"328638","messageId":"5c82e442-9d3a-d748-248d-3ea55a91f777@ramsayjones.plus.com","threadId":"46801","inReplyTo":"20170922052916.fiafkbykroenq65n@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-22T15:46:48Z","receivedAt":"2017-09-22T15:46:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 22/09/17 06:29, Jeff King wrote:\n> On Thu, Sep 21, 2017 at 05:47:36PM +0100, Ramsay Jones wrote:\n> \n>> diff --git a/commit-slab.h b/commit-slab.h\n>> index 333d81e37..dcaab8ca0 100644\n>> --- a/commit-slab.h\n>> +++ b/commit-slab.h\n>> @@ -78,7 +78,7 @@ static MAYBE_UNUSED void init_ ##slabname(struct slabname *s)\t\t\\\n>>  \t\t\t\t\t\t\t\t\t\\\n>>  static MAYBE_UNUSED void clear_ ##slabname(struct slabname *s)\t\t\\\n>>  {\t\t\t\t\t\t\t\t\t\\\n>> -\tint i;\t\t\t\t\t\t\t\t\\\n>> +\tunsigned int i;\t\t\t\t\t\t\t\\\n>>  \tfor (i = 0; i < s->slab_count; i++)\t\t\t\t\\\n>>  \t\tfree(s->slab[i]);\t\t\t\t\t\\\n>>  \ts->slab_count = 0;\t\t\t\t\t\t\\\n>> @@ -89,13 +89,13 @@ static MAYBE_UNUSED elemtype *slabname## _at_peek(struct slabname *s,\t\\\n>>  \t\t\t\t\t\t  const struct commit *c, \\\n>>  \t\t\t\t\t\t  int add_if_missing)   \\\n>>  {\t\t\t\t\t\t\t\t\t\\\n>> -\tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n>> +\tunsigned int nth_slab, nth_slot;\t\t\t\t\\\n> \n> I have a feeling that in the long run these should all be size_t, but\n> it's probably pretty unlikely to overflow in practice. At any rate, the\n> slab index itself is an unsigned, so it probably makes sense to match\n> that for now.\n\nYes, I briefly considered that, but I didn't want to think about\npossible effects of the increased size of 'struct slabname'. I suspect\nthat it is very unlikely to cause an issue, but I had similar concerns\nwith the 'ALLOC_GROW' patch, were it would have been more likely to\ncause memory pressure issues (to e.g. s/int/size_t/). I decided that it\ncould be addressed separately, with a patch on top, if necessary.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"328639","messageId":"20170922155516.wcvpp37usb34ed3i@sigill.intra.peff.net","threadId":"46801","inReplyTo":"5c82e442-9d3a-d748-248d-3ea55a91f777@ramsayjones.plus.com","subject":"Re: [PATCH 2/4] commit-slab.h: avoid -Wsign-compare warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-22T15:55:17Z","receivedAt":"2017-09-22T15:55:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2017 at 04:46:48PM +0100, Ramsay Jones wrote:\n\n> >> -\tint nth_slab, nth_slot;\t\t\t\t\t\t\\\n> >> +\tunsigned int nth_slab, nth_slot;\t\t\t\t\\\n> > \n> > I have a feeling that in the long run these should all be size_t, but\n> > it's probably pretty unlikely to overflow in practice. At any rate, the\n> > slab index itself is an unsigned, so it probably makes sense to match\n> > that for now.\n> \n> Yes, I briefly considered that, but I didn't want to think about\n> possible effects of the increased size of 'struct slabname'. I suspect\n> that it is very unlikely to cause an issue, but I had similar concerns\n> with the 'ALLOC_GROW' patch, were it would have been more likely to\n> cause memory pressure issues (to e.g. s/int/size_t/). I decided that it\n> could be addressed separately, with a patch on top, if necessary.\n\nI _think_ we'd be OK in all of those cases because these size_t's tend\nto be once-per-big-list, not once-per-list-item. But I agree that it\nwould need looking into closely, and definitely should be addressed\nseparately.\n\nThanks.\n\n-Peff\n"}]}