This refactor doesn't change behaviour, but clarifies that the numbers being dealt with aren't supposed to be negative. This helps when reading the code and allows to remove a sanitizer suppression for the whole file.
Fix implicit-integer-sign-change in arith_uint256 #24059
pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2201-int changing 2 files +2 −3-
MarcoFalke commented at 3:00 PM on January 13, 2022: member
-
Fix implicit-integer-sign-change in arith_uint256 fa99e108e7
- MarcoFalke added the label Refactoring on Jan 13, 2022
- PastaPastaPasta approved
-
PastaPastaPasta commented at 12:34 AM on January 14, 2022: contributor
utACK fa99e108e778b5169b3de2ce557af68f1fe0ac0b
pn is a vector of uint32, so we should bitwise or it with uint too nCompact is uint32, so we should bitwise and against unsigned
Both changes make sense. And removing a suppression is good too :)
- shaavan approved
-
shaavan commented at 2:23 PM on January 15, 2022: contributor
ACK fa99e108e778b5169b3de2ce557af68f1fe0ac0b @PastaPastaPasta quite nicely explains the reasoning
pn is a vector of uint32, so we should bitwise or it with uint too nCompact is uint32, so we should bitwise and against unsigned
And I agree with it.
p.s.: I was curious about one thing. Is our ideal goal is to remove the suppression of all the file-wide warnings? Or is it just to remove the redundant ones?
- MarcoFalke merged this on Jan 17, 2022
- MarcoFalke closed this on Jan 17, 2022
-
MarcoFalke commented at 7:49 AM on January 17, 2022: member
p.s.: I was curious about one thing. Is our ideal goal is to remove the suppression of all the file-wide warnings? Or is it just to remove the redundant ones?
Not sure if it makes sense to remove all of them, but where it makes sense, they may be removed.
- MarcoFalke deleted the branch on Jan 17, 2022
- sidhujag referenced this in commit 99b5ce35f8 on Jan 18, 2022
- Fabcien referenced this in commit 02c21f2efd on Dec 9, 2022
- DrahtBot locked this on Jan 17, 2023
Contributors
Labels