all: print unsigned values with unsigned conversions - #410
Open
xanimo wants to merge 1 commit into
Open
Conversation
cppcheck reports eighteen invalidPrintfArgType_sint findings: %d used
with unsigned arguments across block.c, headersdb_file.c, spv.c,
validation.c, wallet.c, such.c, spvnode.c and tool.c.
None of them misbehave for the values these actually carry -- a height or
a merkle index would have to exceed INT_MAX to print wrong -- so this is
correctness rather than a bug fix. It is worth doing anyway, because
these eighteen are most of what stands between the cppcheck gate and the
warning level its own header says it should reach:
Gating widens to `warning` once the initial backlog is dispositioned
and because a suppression would have been the wrong way to clear them.
The suppressions file is for false positives with a written reason; these
are real, just harmless.
uint32_t fields use PRIu32 rather than %u. The tree already uses the PRI
macros in koinu.c, net.c and wallet.c, and it states the width at the
call site instead of relying on unsigned int and uint32_t happening to
agree. Plain unsigned int loop counters use %u. inttypes.h is added where
PRIu32 is now used and was not already included.
dogecoin_bool arguments were left alone: it is a uint8_t, which promotes
to int, so %d is already correct for those and cppcheck does not flag
them.
Local cppcheck goes from 34 warnings to 16, and the remainder is covered
by work already open: the null-check findings by dogecoinfoundation#404, the qr.c
negativeIndex and bip39 invalidscanf by dogecoinfoundation#401, and the OP-TEE uninitvar
false positives by dogecoinfoundation#403's suppressions. The internalAstError hits are
cppcheck 2.7 parser limits and do not appear on the 2.13 the CI image
ships.
81/81.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cppcheck reports eighteen
invalidPrintfArgType_sintfindings —%dused with unsigned arguments acrossblock.c,headersdb_file.c,spv.c,validation.c,wallet.c,such.c,spvnode.candtool.c.None of them misbehave for the values these actually carry. A height or a merkle index would have to exceed
INT_MAXto print wrong. So this is correctness, not a bug fix, and I'd rather say that up front than dress it up.It's worth doing anyway for two reasons.
It's most of what stands between the cppcheck gate and the level its own header says it should reach:
And a suppression would have been the wrong tool. The suppressions file is for false positives with a written reason. These are real findings — just harmless ones. Suppressing them is how a gate quietly stops meaning anything.
Choices
uint32_tfields usePRIu32rather than%u. The tree already uses the PRI macros inkoinu.c,net.candwallet.c, and it states the width at the call site instead of relying onunsigned intanduint32_thappening to agree. Plainunsigned intloop counters use%u.inttypes.hadded wherePRIu32is now used and wasn't already included.dogecoin_boolarguments were deliberately left alone — it's auint8_t, which promotes toint, so%dis already correct there, and cppcheck doesn't flag them. Changing those would have been noise.Where this leaves the backlog
Local cppcheck: 34 warnings → 16. The remainder is already covered by open work:
nullPointerRedundantCheck+ 2ctunullpointernegativeIndex(qr.c), 1invalidscanf(bip39)uninitvar(OP-TEE false positives)internalAstErrorSo with #401, #403, #404 and this, the phase-1 backlog is essentially clear and the gate can widen to
warningas intended.81/81.