Re: [PATCH 2/9] commit-graph: stop using signed integers to count bloom filters

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2025, 18:34 UTC
Message-ID: <xmqq5xf35429.fsf@gitster.g>
In-Reply-To: <aJCW7aYqJB20LDNg@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 12 quoted lines

> On Mon, Aug 04, 2025 at 11:13:28AM +0200, Oswald Buddenhagen wrote:
>> On Mon, Aug 04, 2025 at 10:17:18AM +0200, Patrick Steinhardt wrote:
>> > When writing a new commit graph we have a couple of counters that
>> > provide statistics around what kind of bloom filters we have or have not
>> > written. These counters naturally count from zero and are only ever
>> > incremented, but they use a signed integer as type regardless.
>> > 
>> > Refactor those fields to be of type `size_t` instead.
>> > 
>> mind elaborating on that choice?
>
> We tend to use `size_t` when counting stuff.

And I would have to say that it is wrong and we need to wean ourselves from such a superstition. Unless you are measuring how big a memory block you ask from the allocator, the platform natural integer is often the right type to do the counting.

Each of your "stuff" may weigh N megabytes in core, and if you have M of them, you may have to ask (N*2**20)*M bytes of memory from the allocator. Your (N*2**20)*M must fit size_t _and_ you must compute it without overflowing or wrapping around.

None of the above mean you have to express N in size_t, though. And more importantly, nobody gives you any extra guarantee that you would compute the result correctly if you used size_t. You can write the right code with platform natural integer, and you have to take the same care (e.g. by using st_mult()) to catch integer overflows even if you used size_t.

> ... Regarding the data size I
> don't really think that matters much. It's not like we have hundreds of
> thousands of commit graphs in-memory at any point in time.

Aren't you saying that a platform natural integer is a much better fit?

As to signedness, it sometimes is better for a struct member that is used to record the number of "stuff" you have to be a signed integer that is initialized to -1 to signal "we haven't counted so we do not yet know how many there are". So

    These counters naturally count from zero and are only ever
    incremented.

is not always a valid excuse to insist that such a variable must be unsigned.

In short, not all but much of the recent "use size_t" topics are misguided, and -Wsign-compare is usually a wrong thing to rely on.