fix(scripts): make lint-cppcheck-dash actually report warnings - #7535
fix(scripts): make lint-cppcheck-dash actually report warnings#7535PastaPastaPasta wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (90)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (86)
WalkthroughThe pull request updates cppcheck tooling and suppressions, replaces manual loops with C++20 ranges algorithms, adds explicit default initialization, and marks overridden destructors. It also simplifies APIs by removing unused parameters, adding Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🕓 Ready for review — 14 ahead in queue (commit 26aafd0) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53576af0a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if re.search(fatal_regexp, line): | ||
| warnings.append(line) | ||
| continue |
There was a problem hiding this comment.
When cppcheck is killed (for example by OOM during the exhaustive parallel run) or encounters another failure whose output lacks these four strings, dependencies_output.returncode is ignored and warnings can remain empty, so the linter exits successfully without analyzing anything. This is the same vacuous-success mode the change intends to prevent; I reproduced it with a fake cppcheck that exits 137 with no output, for which this script returned 0. Treat any nonzero subprocess return code as a lint failure in addition to parsing fatal diagnostics.
Useful? React with 👍 / 👎.
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The cppcheck preprocessing fix, version upgrade, warning filtering, and associated warning cleanups appear sound. However, the linter still ignores cppcheck's process exit status, so analyzer failures outside the small recognized-message list can silently pass, contradicting the PR's goal of preventing vacuous analysis.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/lint/lint-cppcheck-dash.py`:
- [BLOCKING] test/lint/lint-cppcheck-dash.py:132-137: Nonzero cppcheck exits still pass the lint
The completed process's `returncode` is never checked. A top-level cppcheck failure that does not emit one of the four recognized `FATAL_ERRORS` strings—such as SIGKILL/OOM, an assertion with different wording, a configuration-loading failure, or an unsupported option—is filtered out and the script exits successfully when no recognized warnings remain. This was reproduced with a cppcheck shim that prints `cppcheck: error: failed to load configuration` and exits 2; the linter exits 0. Because ordinary cppcheck findings do not produce a nonzero status unless `--error-exitcode` is configured, failing on any nonzero status preserves the existing warning filtering while ensuring analyzer failures cannot silently pass.
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/blockchain.cpp`:
- Line 568: Update TimestampIndex::ReadRange and GetBlockHashes to return a
boolean success status, propagating pcursor->Status() failures instead of
silently using partial blockHashes. In the getblockhashes RPC flow around
g_timestampindex->GetBlockHashes, check the returned status and raise
RPC_MISC_ERROR before constructing the successful result.
In `@test/lint/lint-cppcheck-dash.py`:
- Around line 128-132: Update the diagnostic filter in the cppcheck parsing
logic to retain the supported non-noise severities: error, warning, style,
performance, and portability. Continue discarding information, note, and
source-context lines before suppression matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cdb36e58-3397-4802-b0c9-cff23cdeb7a0
📒 Files selected for processing (88)
contrib/containers/ci/ci-slim.Dockerfilesrc/active/context.hsrc/active/dkgsession.cppsrc/active/dkgsession.hsrc/active/dkgsessionhandler.hsrc/bench/bls_dkg.cppsrc/bls/bls.cppsrc/bls/bls.hsrc/bls/bls_worker.cppsrc/chainlock/chainlock.hsrc/chainlock/signing.cppsrc/chainlock/signing.hsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/coinjoin/util.cppsrc/coinjoin/util.hsrc/coinjoin/walletman.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/dmn_types.hsrc/evo/mnhftx.hsrc/evo/netinfo.cppsrc/evo/netinfo.hsrc/evo/providertx.cppsrc/evo/simplifiedmns.cppsrc/evo/smldiff.cppsrc/evo/specialtxman.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/signing.cppsrc/governance/superblock.cppsrc/governance/superblock.hsrc/index/addressindex.cppsrc/index/addressindex.hsrc/index/addressindex_types.hsrc/index/timestampindex.cppsrc/index/timestampindex.hsrc/init.cppsrc/instantsend/instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/llmq/commitment.cppsrc/llmq/core_write.cppsrc/llmq/debug.cppsrc/llmq/dkgmessages.hsrc/llmq/dkgsessionmgr.hsrc/llmq/ehf_signals.hsrc/llmq/net_dkg.cppsrc/llmq/net_dkg.hsrc/llmq/net_quorum.cppsrc/llmq/net_quorum.hsrc/llmq/net_signing.cppsrc/llmq/observer.hsrc/llmq/params.hsrc/llmq/quorums.hsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/llmq/snapshot.hsrc/llmq/utils.cppsrc/masternode/meta.hsrc/node/interfaces.cppsrc/qt/clientfeeds.hsrc/qt/donutchart.hsrc/rpc/blockchain.cppsrc/rpc/coinjoin.cppsrc/rpc/evo.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cppsrc/stacktraces.cppsrc/stats/client.cppsrc/test/evo_netinfo_tests.cppsrc/test/util/llmq_tests.hsrc/util/ranges_set.cppsrc/util/ranges_set.hsrc/util/std23.hsrc/wallet/bip39.cppsrc/wallet/coinjoin.cppsrc/wallet/hdchain.cppsrc/wallet/hdchain.htest/lint/lint-cppcheck-dash.py
💤 Files with no reviewable changes (2)
- src/governance/net_governance.cpp
- src/active/dkgsession.cpp
| # 'note:' and source-context lines only make sense next to their parent | ||
| # warning; on their own (e.g. when the parent is suppressed) they are | ||
| # noise, and they don't carry the check id the suppressions match on. | ||
| if ' warning: ' not in line and ' error: ' not in line: | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate lint-cppcheck-dash.py"
fd -a 'lint-cppcheck-dash.py' .
echo
echo "Relevant file excerpt"
if [ -f test/lint/lint-cppcheck-dash.py ]; then
nl -ba test/lint/lint-cppcheck-dash.py | sed -n '1,220p'
fi
echo
echo "Search for cppcheck/GCC and suppression handling symbols"
rg -n "cppcheck|ALWAYS_ENABLED_WARNINGS|SUPPRESSED_WARNINGS|GCC|style|performance|portability|note:" test/lint/lint-cppcheck-dash.py src test -S || true
echo
echo "Check for cppcheck canary/fixtures/tests"
rg -n "cppcheck-dash|lint-cppcheck|cppcheck" test/lint/LintRunner.py test/lint test .github ci 2>/dev/null || trueRepository: dashpay/dash
Length of output: 294
🌐 Web query:
Cppcheck report output GCC format information warning error style performance portability note severity
💡 Result:
To generate GCC-compatible report output in Cppcheck, you should use the --template=gcc flag [1][2]. This formats the error messages to match the standard GCC diagnostic output format [1][2]. Cppcheck categorizes findings into specific severity levels, which can be identified in reports [3][4]: 1. Error: Indicates a severe issue, such as a memory leak or other certain programming error [3][4]. 2. Warning: Used for dangerous coding patterns that may lead to runtime errors (e.g., uninitialized member variables) [3][4]. 3. Style: General recommendations for code cleanup, such as redundant or unreachable code; these improve maintainability rather than fixing bugs [3][4]. 4. Performance: Suggests suboptimal code that, if corrected, may result in better performance [3][4]. 5. Portability: Highlights code that is not portable across different platforms or bitnesses (e.g., 32-bit vs. 64-bit systems) [3][4]. 6. Information: Informational messages about the checking process itself, such as missing header files [3][4]. 7. Debug: Messages intended for developer use [4]. You can suppress specific warnings using the --suppress flag, which follows the format [error id]:[filename]:[line] [5][6]. The warning ID is typically found in square brackets at the end of the error message provided by Cppcheck (e.g., [arrayIndexOutOfBounds]) [1][7]. Alternatively, you can use inline suppressions by placing // cppcheck-suppress warningId above the relevant code line [7].
Citations:
- 1: https://github.com/danmar/cppcheck/blob/main/man/manual.md
- 2: https://cppcheck.sourceforge.io/manual.pdf
- 3: https://cppcheck.sourceforge.io/devinfo/doxyoutput/classSeverity.html
- 4: https://github.com/danmar/cppcheck/blob/d69955c0/lib/errortypes.h
- 5: https://manpages.ubuntu.com/manpages/focal/man1/cppcheck.1.html
- 6: https://manpages.ubuntu.com/manpages/noble/man1/cppcheck.1.html
- 7: https://docs.platformio.org/en/latest/advanced/static-code-analysis/tools/cppcheck.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant file excerpt"
awk '{printf "%6d %s\n", NR, $0}' test/lint/lint-cppcheck-dash.py | sed -n '1,260p'
echo
echo "Search for cppcheck handling symbols"
grep -RInE "cppcheck|ALWAYS_ENABLED_WARNINGS|SUPPRESSED_WARNINGS|GCC|style|performance|portability|information|note" test/lint/lint-cppcheck-dash.py test/lint test .github ci README.md 2>/dev/null || true
echo
echo "Behavioral probe of current line-filtering slice"
python3 - <<'PY'
from pathlib import Path
src = Path('test/lint/lint-cppcheck-dash.py').read_text()
idx = src.find("if ' warning: ' not in line and ' error: ' not in line:")
print("current_guard_find", idx)
for severity in ["error:", "warning:", "style:", "performance:", "portability:", "information:", "note:"]:
line = f"{__file__}:123: {severity} message [undefMacro]"
print(severity, bool(" warning: " in line or " error: " in line))
PY
echo
echo "Check installed cppcheck availability/version"
if command -v cppcheck >/dev/null 2>&1; then
cppcheck --version || true
else
echo "cppcheck not installed"
fiRepository: dashpay/dash
Length of output: 20273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Build a minimal C++ file emitting different cppcheck GCC severities"
tmpdir="$(mktemp -d)"
cat > "$tmpdir/test.cpp" <<'CPP'
// cppcheck:style:Redundant initialization.
char redundantInitialized = 0;
// cppcheck:performance:Use of unary operator not needed.
void unary() {
int x = +1;
}
// cppcheck:portability:Bit shift operand has undefined behavior.
void shift() {
unsigned u = 1;
volatile unsigned r = u << 32;
}
// cppcheck:information:Missing preprocessor directive explanation.
int missingInclude;
void noteParent() {
int *p = nullptr;
// cppcheck-suppress nullPointerRedundantCheck
*p = 1; // cppcheck:note:pointer p might be null.
}
CPP
echo
echo "cppcheck output"
cppcheck --version
cppcheck --enable=all --style --performance --portability --information --inline-suppr --template=gcc -I "$tmpdir" "$tmpdir/test.cpp" 2>&1 || true
echo
echo "Filter simulation"
python3 - <<'PY'
out = """
/home/jailuser/git/test.cpp:1:5: style: Redundant initialization. The variable 'redundantInitialized' is assigned a value that is never used. [redu ndantNextlineCheck]
/home/jailuser/git/test.cpp:6:5: performance: Unary operator '+' is redundant.
/home/jailuser/git/test.cpp:11:15: portability: Bit shift operand has undefined behavior.
/home/jailuser/git/test.cpp:16:1: information: Missing include header explanation.
/home/jailuser/git/test.cpp:22:5: note: pointer p might be null.
"""
for line in out.strip().splitlines():
print(repr(line), bool(" warning: " in line or " error: " in line))
PYRepository: dashpay/dash
Length of output: 857
🌐 Web query:
Cppcheck 2.21 --template=gcc output format style warning information severity line
💡 Result:
When using Cppcheck with the --template=gcc option, the output follows a format compatible with GCC compiler messages, which is designed for integration with build systems and IDEs [1][2][3]. The output format generated by --template=gcc is: {file}:{line}:{column}: {severity}: {message} [{id}] For example, a typical output line looks like this: samples/arrayIndexOutOfBounds/bad.c:6:6: warning: Array 'a[2]' accessed at index 2, which is out of bounds. [arrayIndexOutOfBounds] Key components of this format include: - {file}: The path to the source file where the issue was detected [2]. - {line}: The line number where the issue occurs [2]. - {column}: The column number within the line [2]. - {severity}: The classification of the finding (e.g., warning, error, style, performance, portability, or information) [2]. - {message}: The descriptive text explaining the detected issue [2]. - {id}: The unique identifier (warning ID) for the specific check [2]. This predefined template provides a standard, machine-parsable structure [3]. If you require a custom format, you can define your own template using the --template option with supported specifiers such as {file}, {line}, {column}, {severity}, {message}, and {id} [2][3].
Citations:
- 1: https://github.com/danmar/cppcheck/blob/main/man/manual.md
- 2: https://cppcheck.sourceforge.io/manual.pdf
- 3: https://cppcheck.sourceforge.io/manual.html
🌐 Web query:
Cppcheck 2.21 --errorlist style performance portability information available command line options
💡 Result:
In Cppcheck 2.21, the categories style, performance, portability, and information are defined as severity levels for diagnostic messages [1][2]. These can be enabled or disabled via command-line options [3][4]. Severities Overview: - style: Used for stylistic issues such as redundant code, unused functions, operator precedence, and constness [1][5]. - performance: Used for runtime performance suggestions based on general knowledge; fixing these does not guarantee measurable speed improvements [1][5]. - portability: Used for implementation-defined behavior, 64-bit portability issues, and certain types of undefined behavior that may work as intended on specific platforms [1][5]. - information: Used for configuration problems that do not relate to the syntactical correctness of the code but indicate that the Cppcheck configuration could be improved [1][2]. Command-Line Options: - --enable=: Enables additional checks. You can enable these categories individually or in combination (e.g., --enable=style,performance) [3][4]. - --enable=style: This specific option enables all coding style checks, which includes messages with the severities 'style', 'performance', and 'portability' [3][4]. - --errorlist: Prints a list of all possible error messages in XML format to the console [6][3][7]. This is the authoritative command for identifying all checks available in your installed version of Cppcheck [6]. For output formatting, the --template option allows you to define custom message formats using placeholders such as {severity}, {message}, {file}, and {line} [1][2][3]. For example, --template='{file}:{line},{severity},{id},{message}' can be used to structure the report [3]. When --xml is used, these severity levels are included as attributes in the XML output [1][2].
Citations:
- 1: https://cppcheck.sourceforge.io/manual.pdf
- 2: https://github.com/danmar/cppcheck/blob/main/man/manual.md
- 3: https://man.archlinux.org/man/cppcheck.1.en
- 4: https://man.archlinux.org/man/cppcheck.1.en.txt
- 5: https://cppcheck.sourceforge.io/manual.html
- 6: https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/
- 7: https://github.com/danmar/cppcheck/blob/main/man/cppcheck.1.xml
Keep valid non-noise cppcheck diagnostic severities.
The script runs cppcheck --enable=all --template=gcc, whose GCC-formatted diagnostics use severities such as error, warning, style, performance, portability, and information. The current filter leaves only warning and error, so style, performance, portability, and information findings are discarded before suppression checking. Match the supported non-noise severities explicitly, and continue discarding only information, note, and source-context lines.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 132-132: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(files_regexp, line)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/lint/lint-cppcheck-dash.py` around lines 128 - 132, Update the
diagnostic filter in the cppcheck parsing logic to retain the supported
non-noise severities: error, warning, style, performance, and portability.
Continue discarding information, note, and source-context lines before
suppression matching.
d802b17 to
b44f9e3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44f9e3217
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # These must always fail the lint (regardless of which file they point at), | ||
| # otherwise analysis silently ends up vacuous. | ||
| FATAL_ERRORS = ( | ||
| "preprocessorErrorDirective", |
There was a problem hiding this comment.
When cppcheck aborts a translation unit with an error such as src/validation.h:...: error: syntax error [syntaxError], this list does not classify it as fatal, and the later file filter discards it because the included header is outside non-backported.txt. The linter can therefore pass after cppcheck skipped analysis of one or more targeted files; include syntaxError and other analysis-abort diagnostics in the path-independent fatal set.
AGENTS.md reference: AGENTS.md:L48-L50
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Two in-scope blockers remain at exact head b44f9e3. PF-1 is still valid because cppcheck's process status is ignored; separately, the latest suppression burn-down re-enables useStlAlgorithm while the diagnostic filter discards its style-severity output, so the check is not actually enforced.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/lint/lint-cppcheck-dash.py`:
- [BLOCKING] test/lint/lint-cppcheck-dash.py:122-127: Nonzero cppcheck exits still pass the lint
The completed process is captured, but dependencies_output.returncode is never inspected. FATAL_ERRORS only catches four textual markers, so a command-line/configuration failure, crash, OOM termination, or other unsuccessful cppcheck exit can produce no retained warning and let the script exit successfully without completing analysis. This reproduces on the current head with a cppcheck shim that exits 2 after version detection: lint-cppcheck-dash.py exits 0. Because the command does not use --error-exitcode, ordinary diagnostics do not make cppcheck return nonzero, so every nonzero process status can safely be treated as an analyzer failure.
- [BLOCKING] test/lint/lint-cppcheck-dash.py:134-138: The diagnostic filter discards cppcheck style findings
The GCC-formatted output filter retains only error and warning severities. Cppcheck classifies both unreadVariable and the useStlAlgorithm check re-enabled by the head commit as style, so these diagnostics are discarded before the allowlist and suppression logic runs. A current-head shim emitting a style-severity unreadVariable diagnostic for a targeted file makes the script exit 0, and cppcheck's error list confirms that useStlAlgorithm and unreadVariable have style severity. Performance and portability findings enabled by --enable=all are also silently discarded.
| # 'note:' and source-context lines only make sense next to their parent | ||
| # warning; on their own (e.g. when the parent is suppressed) they are | ||
| # noise, and they don't carry the check id the suppressions match on. | ||
| if ' warning: ' not in line and ' error: ' not in line: | ||
| continue |
There was a problem hiding this comment.
🔴 Blocking: The diagnostic filter discards cppcheck style findings
The GCC-formatted output filter retains only error and warning severities. Cppcheck classifies both unreadVariable and the useStlAlgorithm check re-enabled by the head commit as style, so these diagnostics are discarded before the allowlist and suppression logic runs. A current-head shim emitting a style-severity unreadVariable diagnostic for a targeted file makes the script exit 0, and cppcheck's error list confirms that useStlAlgorithm and unreadVariable have style severity. Performance and portability findings enabled by --enable=all are also silently discarded.
| # 'note:' and source-context lines only make sense next to their parent | |
| # warning; on their own (e.g. when the parent is suppressed) they are | |
| # noise, and they don't carry the check id the suppressions match on. | |
| if ' warning: ' not in line and ' error: ' not in line: | |
| continue | |
| # Keep primary diagnostics while discarding information, notes, and source context. | |
| if not re.search(r' (?:error|warning|style|performance|portability): ', line): | |
| continue |
source: ['codex']
|
This pull request has conflicts, please rebase. |
b44f9e3 to
97e975e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 97e975e, both carried-forward blockers remain reproducible: nonzero cppcheck exits can pass, and style diagnostics are discarded before suppression handling. No genuinely new latest-delta findings were identified; the timestamp-index hardening suggested by CodeRabbit concerns behavior that already existed at the PR base and is outside this lint-focused change.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/lint/lint-cppcheck-dash.py`:
- [BLOCKING] test/lint/lint-cppcheck-dash.py:122-127: Nonzero cppcheck exits still pass the lint
The completed cppcheck process is captured, but its return code is never checked. An analyzer crash, invalid invocation, OOM termination, or other unsuccessful exit can therefore pass whenever its output does not match one of the four FATAL_ERRORS patterns and survive the later file/severity filters. At the current head, a shim that succeeds for `--version`, prints an unrecognized configuration error, and exits 2 makes this linter exit 0. Because the command does not set `--error-exitcode`, ordinary cppcheck diagnostics retain the default zero status, so treating every nonzero status as an analyzer failure does not conflate findings with execution failures.
| { | ||
| CBLSIdImplicit() = default; | ||
| // cppcheck-suppress noExplicitConstructor | ||
| CBLSIdImplicit(const uint256& id) |
There was a problem hiding this comment.
why don't add explicit here? any good reason for it?
Fix is trivial:
diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp
index 49981ee919..884523fb47 100644
--- a/src/bls/bls.cpp
+++ b/src/bls/bls.cpp
@@ -27,7 +27,7 @@ static const std::unique_ptr<bls::CoreMPL>& Scheme(const bool fLegacy)
CBLSId::CBLSId(const uint256& nHash) : CBLSWrapper<CBLSIdImplicit, BLS_CURVE_ID_SIZE, CBLSId>()
{
- impl = nHash;
+ impl = CBLSIdImplicit{nHash};
fValid = true;
cachedHash.SetNull();
}
diff --git a/src/bls/bls.h b/src/bls/bls.h
index 02a3bbefa5..dba9f198a2 100644
--- a/src/bls/bls.h
+++ b/src/bls/bls.h
@@ -234,7 +234,7 @@ public:
struct CBLSIdImplicit : public uint256
{
CBLSIdImplicit() = default;
- CBLSIdImplicit(const uint256& id)
+ explicit CBLSIdImplicit(const uint256& id)
{
memcpy(begin(), id.begin(), sizeof(uint256));
}There was a problem hiding this comment.
Deferring making CBLSIdImplicit explicit to a follow-up PR. The intentional implicit conversion is still used at the CBLSId construction site (impl = nHash), and tightening that is a small but separate API-style change that can be done with its own call-site audit.
| auto ret = CGovernanceObject::GetStateJsonHelp(/*key=*/"", /*optional=*/false, /*local_valid_key=*/"fBlockchainValidity"); | ||
| auto mod_inner = ret.m_inner; | ||
| for (const auto& result : CGovernanceObject::GetVotesJsonHelp(/*key=*/"", /*optional=*/false).m_inner) { | ||
| // The range expression's temporary is lifetime-extended for the whole loop |
There was a problem hiding this comment.
Isn't it safe for C++23 only? See proposal P2718
Should be fixed instead suppressed then.
There was a problem hiding this comment.
Agreed — the lifetime-extension of a range-for temporary is only guaranteed by P2718/C++23. That path is no longer relying on the temporary: useStlAlgorithm stores the helper result in a named local (votes_help) and iterates that, so no suppression is needed anymore.
|
|
||
| { | ||
| auto& db = sigman.GetDb(); | ||
| auto& db = signing_manager.GetDb(); |
There was a problem hiding this comment.
auto& db = sigman.GetDb();
auto& db = signing_manager.GetDb();
how exactly it works? It seems as some changes for lint: re-enable shadowMember cppcheck wrongly squashed to other commit.
| if (!opt_dsq.has_value()) return false; | ||
|
|
||
| connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::DSQUEUE, *opt_dsq)); | ||
| connman_in.PushMessage(&pfrom, msgMaker.Make(NetMsgType::DSQUEUE, *opt_dsq)); |
There was a problem hiding this comment.
how exactly it works? It seems as some changes for lint: re-enable shadowMember cppcheck wrongly squashed to other commit.
| */ | ||
| bool ConfirmInventoryRequest(const CInv& inv) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_store); | ||
| bool ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) |
There was a problem hiding this comment.
should remove also class CConnman; forward declaration
There was a problem hiding this comment.
Done in 4c40ab4 — the unused class CConnman; forward declaration is removed from governance.h.
| vote.SetSignature(m_mn_activeman.SignBasic(vote.GetSignatureHash())); | ||
|
|
||
| CGovernanceException exception; | ||
| if (!m_govman.ProcessVoteAndRelay(vote, exception, m_connman)) { |
There was a problem hiding this comment.
should remove member variable CConnman& m_connman; and forward declaration from signing.h also
There was a problem hiding this comment.
Done in 4c40ab4: dropped m_connman (and the constructor parameter / call site in ActiveContext) from GovernanceSigner, since ProcessVoteAndRelay no longer needs a CConnman and the member was unused.
| } | ||
|
|
||
| CConnman& connman = EnsureConnman(node); | ||
| EnsureConnman(node); |
There was a problem hiding this comment.
no Connman here is needed at all
-EnsureConnman(node);There was a problem hiding this comment.
Removed in 4c40ab4. ProcessVoteAndRelay only queues relay inventory and no longer needs CConnman, so the dead EnsureConnman(node) call is gone.
| CGovernanceException exception; | ||
| CConnman& connman = EnsureConnman(node); | ||
| if (node.govman->ProcessVoteAndRelay(vote, exception, connman)) { | ||
| EnsureConnman(node); |
There was a problem hiding this comment.
no Connman here is needed at all:
-EnsureConnman(node);| const CBlockIndex* pBaseBlockIndex = ParseBlockIndex(request.params[0], chainman, "baseBlock"); | ||
| const CBlockIndex* pTargetBlockIndex = ParseBlockIndex(request.params[1], chainman, "block"); | ||
|
|
||
| if (pBaseBlockIndex == nullptr) { |
There was a problem hiding this comment.
nit: make ParseBlockIndex to return gsl::non_null to enforce that it won't be null ptr ;
There was a problem hiding this comment.
Fair nit, but out of scope for this cppcheck burn-down PR. Happy to take gsl::not_null for ParseBlockIndex in a follow-up if we want to enforce the post-condition more strongly.
The cppcheck linter has been silently analyzing nothing (see next commit), letting several warnings in non-backported files accumulate. Fix the ones that the linter's ALWAYS_ENABLED_WARNINGS patterns force-report: remove unused/dead locals, make single-argument constructors explicit (with an inline suppression for CBLSIdImplicit, whose implicit conversion is intentional), narrow benchmark counters to the scope they are used in, pass CSigBase and BlsCheck constructor arguments by reference/move, and inline-suppress a danglingTempReference false positive on a lifetime-extended range-for temporary.
The linter has been vacuous in two ways. First, without __GNUC__ defined, src/attributes.h hits '#error No known always_inline attribute', which aborts cppcheck's analysis of nearly every translation unit; the resulting preprocessorErrorDirective lines were then dropped by the output filter because they don't point at files from non-backported.txt. Second, even with preprocessing fixed, cppcheck 2.17.1 crashes with an assertion in TokenList::setLang under --check-level=exhaustive, and that crash was explicitly suppressed. Define __GNUC__ so preprocessing succeeds, bump cppcheck to 2.21.0 (which no longer crashes with exhaustive checking) and drop the crash suppression, and treat analysis failures (preprocessorErrorDirective, internal errors) as lint failures regardless of which file they point at so the linter can never silently go vacuous again. Filter out 'note:'/source-context lines, which don't carry the check id that suppressions match on and would leak through when their parent warning is suppressed. Finally, suppress the check ids with pre-existing violations in the tree so the linter can be enforced; these should be burned down and re-enabled over time.
The shadowMember re-enable left dangling parameter names after dashpay#7538/dashpay#7539 dropped those parameters (connman_in / signing_manager), which broke the build. Finish the ProcessVoteAndRelay cleanup by removing the now-unused CConnman plumbing from GovernanceSigner, governance RPC helpers, and node interfaces. Cap cppcheck jobs and treat abnormal cppcheck exits as failures so CI analysis cannot pass vacuous when the child process dies.
4c40ab4 to
26aafd0
Compare
3 participants
Issue being fixed or feature implemented
test/lint/lint-cppcheck-dash.pyhas been effectively vacuous — it ran cppcheck, silently analyzed almost nothing, and always passed. Two failure modes:-Dlist,__GNUC__is not defined, sosrc/attributes.hhits#error No known always_inline attribute(preprocessorErrorDirective), aborting cppcheck's analysis of nearly every translation unit. The error lines were then dropped by the output filter, which only keeps lines pointing at files fromtest/util/data/non-backported.txt.--check-level=exhaustivecrashes with an assertion inTokenList::setLang(child dies with signal 6) — and that crash message was explicitly listed inSUPPRESSED_WARNINGS.Net effect: an injected canary warning in a non-backported file was not flagged, locally or in CI.
What was done?
Linter (
test/lint/lint-cppcheck-dash.py):__GNUC__so preprocessing succeeds.FATAL_ERRORSlist (preprocessorErrorDirective, internal errors/crashes) that fails the lint regardless of which file the line points at, so analysis failures can never again be silently filtered away.note:/source-context lines: they don't carry the check id that suppressions match on, so orphaned notes of suppressed warnings leaked through the filter.knownConditionTrueFalse,shadowFunction,uninitMemberVarNoCtor, etc.), documented as a burn-down TODO, so the linter can be enforced now. Messages matchingALWAYS_ENABLED_WARNINGSstill override these suppressions.Container (
contrib/containers/ci/ci-slim.Dockerfile): bump cppcheck 2.17.1 → 2.21.0, which no longer crashes under--check-level=exhaustive.Code fixes for the ~11 warnings that
ALWAYS_ENABLED_WARNINGSpatterns force-report (these cannot be suppressed by check id): removed dead locals (src/active/dkgsession.cpp,src/rpc/evo.cpp), made single-argument constructorsexplicit(chainlock::Chainlocks,CDSTXManager; no implicit-conversion call sites exist), inline-suppressedCBLSIdImplicit's intentionally implicit constructor, narrowed three static benchmark counters to their usage scope insrc/evo/specialtxman.cpp(static storage duration unchanged), passedCSigBaseby const reference inInitSession(callers already sliced identically by value; accessors are non-virtual), movedBlsCheck's by-value constructor parameters into members, and inline-suppressed adanglingTempReferencefalse positive on a lifetime-extended range-for temporary insrc/rpc/governance.cpp.How Has This Been Tested?
With cppcheck 2.21.0 locally (matching the bumped container version):
unreadVariable) intosrc/spork.cpp: the linter reports exactly that warning and exits 1.-D__GNUC__to simulate failure mode 1: theattributes.h#errorline surfaces viaFATAL_ERRORSand the linter exits 1.clang -fsyntax-onlywith the project's compile flags;test/lint/lint-python.pypasses.Note: the previously-suppressed
count_if/find_ifuseStlAlgorithmvariants are now covered by the wholesale id suppression along with the rest of the burn-down list.Breaking Changes
None. CI lint may take somewhat longer since cppcheck now actually analyzes all 273 non-backported files with
--check-level=exhaustive.Checklist: