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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 06, 2025 at 08:41:32AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> I wrote these counters in 312cff5207 (bloom: split 'get_bloom_filter()'
> >> in two, 2020-09-16) and 59f0d5073f (bloom: encode out-of-bounds filters
> >> as non-empty, 2020-09-17), and I don't see a compelling reason that
> >> these should be unsigned.
> >
> > I think that is going backwards though: the question to ask is why
> > should these be signed if they cannot ever be negative?
> 
> Earlier I gave an example of allowing for a "not yet counted"
> sentinel value for a variable or a structure member.  Another
> example may be for a function that counts that also needs to signal
> an error, and as usual in any C programs, the natural way to do so
> for any function whose "normal" return values are non-negative
> integers is to signal errors with a negative value.
> 
> Note that a structure member or a variable that does not need such a
> "not yet counted" sentinel value (e.g., it may have a separate
> "counted already" member associated with it, or the nature of the
> thing it counts does not have such "not yet counted" state), and it
> is possible for such a variable to live happily with a function that
> can signal an error.
> 
> It means the variable that receives the counted result from such a
> function may be able to use only half a range of values as its type
> implies, if that helper function is the only source of information
> that is assigned to it, though.
> 
> If the counter in question never needs to store such a sentinel
> value itself, then I am OK for it to be unsigned, and that is
> exactly why I said "not always a valid excuse".  But if the counter
> variable or structure member has to work with functions that need to
> return sentinel values (like platform natural int that can use the
> usual "negative is an error, non-negative is a normal result"), it
> may have less chance to trigger the -Wsign-compare irritation, if
> you made it also signed.

Yup, fully agreed, and this is a good reason why it should be signed. In
the case at hand though we never use such sentinel values, and I think
making that explicit by using an unsigned type is a good thing as it
tells the reader that "Yup, no sentinels involved, it's a plain counter
from 0 to $NUM_ENTRIES".

Patrick