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) ...
