Fix implicit-integer-sign-change in bloom #24219

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2201-bloomInt changing 2 files +3 −4

MarcoFalke commented at 4:34 PM on January 31, 2022: member

Signed values don't really make sense when using std::vector::operator[].

Fix that and remove the suppression.
Fix implicit-integer-sign-change in bloom fa041878de
MarcoFalke added the label Refactoring on Jan 31, 2022
MarcoFalke commented at 5:00 PM on January 31, 2022: member

I get the same binary with gcc and clang on my system with -O2.
DrahtBot commented at 8:39 PM on January 31, 2022: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
refactor: Fixup uint64_t-cast style in touched line fad84a2595

in src/common/bloom.cpp:221 in fa041878de outdated

 217 | @@ -218,7 +218,7 @@ void CRollingBloomFilter::insert(Span<const unsigned char> vKey)
 218 |          /* FastMod works with the upper bits of h, so it is safe to ignore that the lower bits of h are already used for bit. */
 219 |          uint32_t pos = FastRange32(h, data.size());
 220 |          /* The lowest bit of pos is ignored, and set to zero for the first bit, and to one for the second. */
 221 | -        data[pos & ~1] = (data[pos & ~1] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration & 1)) << bit;
 222 | +        data[pos & ~1U] = (data[pos & ~1U] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration & 1)) << bit;

PastaPastaPasta commented at 5:07 AM on February 1, 2022:

please :) (otherwise PR looks good)

        data[pos & ~1U] = (data[pos & ~1U] & ~(uint64_t{1} << bit)) | (uint64_t(nGeneration & 1)) << bit;

MarcoFalke commented at 10:35 AM on February 1, 2022:

Thx, done in a new commit

PastaPastaPasta approved
PastaPastaPasta commented at 11:53 AM on February 1, 2022: contributor

utACK fad84a25956ec081f22aebbda309d168a3dc0004
theStack approved
theStack commented at 1:48 PM on February 2, 2022: member

Code-review ACK fad84a25956ec081f22aebbda309d168a3dc0004

Can confirm that the compiled bitcoind binary is unchanged with clang 11.1.0 (Target amd64-unknown-openbsd7.0).
MarcoFalke merged this on Feb 2, 2022
MarcoFalke closed this on Feb 2, 2022
MarcoFalke deleted the branch on Feb 2, 2022
sidhujag referenced this in commit 93b839ec6e on Feb 3, 2022
Fabcien referenced this in commit 6b26906907 on Dec 9, 2022
DrahtBot locked this on Feb 2, 2023

Contributors

Labels

Fix implicit-integer-sign-change in bloom #24219

Conflicts