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
