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

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Wed, Aug 06, 2025 at 08:23:36AM +0200, Patrick Steinhardt wrote:
>On Mon, Aug 04, 2025 at 05:44:06PM -0400, Taylor Blau wrote:
>> 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?
>
>> It's true that we don't have any need for negative values here since we
>> are counting from zero, but I don't think that alone justifies changing
>> the signed-ness here.
>> 
>> Is there a reason beyond "these are always non-negative" that changing
>> the signed-ness is warranted? If so, let's discuss that and make sure
>> that it is documented in the commit message. If not, I think we could
>> drop this patch (and optionally the patch before it as well).
>
>Yes, there is: it makes code easier to reason about for the reader, and
>it means that we don't have to guard ourselves against negative values.
>
>If I see a counting variable that is signed I immediately jump to the
>question of whether or not it can ever be a negative value. I assume
>that the author of this code _intentfully_ made it signed to cater to a
>specific edge case.
>
well, there is also the diametrically opposed view:
https://google.github.io/styleguide/cppguide.html#Integer_Types
https://critical.eschertech.com/2010/04/07/danger-unsigned-types-used-here/
https://soundsoftware.ac.uk/c-pitfall-unsigned
https://stackoverflow.com/questions/30395205/why-are-unsigned-integers-error-prone
..

in isync, i standardized on unsigned where possible (e2d3b4d55), and 
sure enough, i introduced one of those underflow bugs not much later 
(859b7dd7f => 12e30ce56) ...