fix: re-enable cppcheck and bump its version #7042
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughUpdated CPPCHECK_VERSION in contrib/containers/ci/ci-slim.Dockerfile from 2.13.0 to 2.17.1. In src/cachemultimap.h, CacheMultiMap::Erase(const K&) now binds the inner map as a const reference and uses a range-for to erase list entries. In src/evo/netinfo.h, DomainPort::ToStringAddr() now returns const std::string& and explicit destructors for MnNetInfo and ExtNetInfo were removed. In test/lint/lint-cppcheck-dash.py, ENABLED_CHECKS/IGNORED_WARNINGS were renamed to ALWAYS_ENABLED_WARNINGS/SUPPRESSED_WARNINGS, filtering/reporting logic was reworked, and cppcheck is invoked with --check-level=exhaustive. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/lint/lint-cppcheck-dash.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-12-01T18:13:21.314Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Applied to files:
test/lint/lint-cppcheck-dash.py
🪛 Flake8 (7.3.0)
test/lint/lint-cppcheck-dash.py
[error] 107-107: continuation line missing indentation or outdented
(E122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint / Run linters
- GitHub Check: Build container / Build container
🔇 Additional comments (2)
test/lint/lint-cppcheck-dash.py (2)
50-51: LGTM!The new ignored warning pattern for branch analysis limits is appropriate for the cppcheck 2.17.1 update. This informational warning about non-exhaustive branch checking is reasonable to ignore for performance reasons.
120-123: The filtering logic correctly implements the intended whitelist/blacklist behavior.The condition
re.search(enabled_regexp, line) or not re.search(ignored_regexp, line)ensures that whitelisted warnings always appear, while non-whitelisted warnings are filtered through the blacklist. This matches the stated PR objective: non-whitelisted items are now properly filtered by the blacklist instead of being blocked entirely.
test/lint/lint-cppcheck-dash.py
Outdated
| '--debug-warnings', | ||
| # '-q', |
There was a problem hiding this comment.
Fix the indentation issue on the commented line.
The commented line has inconsistent indentation (8 spaces) compared to the line above (4 spaces), causing a flake8 E122 error.
Apply this diff to fix the indentation:
'--debug-warnings',
-# '-q',
+# '-q',Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 107-107: continuation line missing indentation or outdented
(E122)
🤖 Prompt for AI Agents
In test/lint/lint-cppcheck-dash.py around lines 106 to 107, the commented list
entry '"# '-q','" is indented with 8 spaces while the previous line uses
4 spaces, causing flake8 E122; fix it by reducing the indentation of the
commented line to match the previous line (align the comment to 4 spaces) so the
list entries have consistent indentation.
test/lint/lint-cppcheck-dash.py
Outdated
| text=True, | ||
| ) | ||
|
|
||
| print("all output: {dependencies_output.stdout}") |
There was a problem hiding this comment.
Fix the f-string syntax error.
The debug print statement is missing the f prefix, so the variable won't be interpolated. This will print the literal string "all output: {dependencies_output.stdout}" instead of the actual output.
Apply this diff to fix the syntax:
- print("all output: {dependencies_output.stdout}")
+ print(f"all output: {dependencies_output.stdout}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print("all output: {dependencies_output.stdout}") | |
| print(f"all output: {dependencies_output.stdout}") |
🤖 Prompt for AI Agents
In test/lint/lint-cppcheck-dash.py around line 117, the debug print uses a raw
string instead of an f-string so "{dependencies_output.stdout}" won't be
interpolated; change the print call to use an f-string (add the leading f before
the opening quote) so the variable value is printed, e.g. make the line
print(f"all output: {dependencies_output.stdout}").
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/lint/lint-cppcheck-dash.py (1)
40-42: Keep cppcheck internal‑error suppression and TODO aligned with actual cppcheck behaviorAdding the
"warning: Internal error: Child process crashed with signal 6"suppression while enabling--check-level=exhaustivewill hide that cppcheck crashed, and the TODO is a bit vague now that the cppcheck version was bumped in this PR.I’d suggest:
- Making the comment more specific (e.g., reference the exact cppcheck version and/or upstream issue) so it’s clear when it’s safe to remove.
- Optionally, treating a non‑zero
dependencies_output.returncodeas a hard failure even if this message is suppressed, so real cppcheck crashes don’t silently pass the lint step.Also applies to: 97-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/lint/lint-cppcheck-dash.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-12-01T18:13:21.314Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Applied to files:
test/lint/lint-cppcheck-dash.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (1)
test/lint/lint-cppcheck-dash.py (1)
121-123: Whitelist/blacklist combination logic now matches the intended semanticsThis condition effectively means: “for our tracked files, report diagnostics that either match
ENABLED_CHECKSor do not matchIGNORED_WARNINGS,” i.e., the blacklist only filters non‑whitelisted warnings. That aligns with the described fix for the previous intersection bug and looks correct.
test/lint/lint-cppcheck-dash.py
Outdated
| if re.search(enabled_regexp, line) and not re.search(ignored_regexp, line) and re.search(files_regexp, line): | ||
| if not re.search(files_regexp, line): | ||
| continue | ||
| if re.search(enabled_regexp, line) or not re.search(ignored_regexp, line): |
There was a problem hiding this comment.
so what's the point of having ENABLED_CHECKS then?
There was a problem hiding this comment.
also "if it's enabled then we don't care if it's ignored" logic feels wrong
There was a problem hiding this comment.
so what's the point of having ENABLED_CHECKS then?
enabled checks are supposed to be shown always.
Checks that are not matched with any "enabled checks" will be filtered by "ignored checks".
It helps to chase 2 things:
- important messages are never shadowed [enabled_regexp]
- non-important messages are filtered for noisy input. [ignored_regexp]
There was a problem hiding this comment.
Let's maybe rename accordingly then e.g. enabled -> important or enforced or whatever. @coderabbitai can you suggest a better name for ENABLED_CHECKS considering the way it's used now?
There was a problem hiding this comment.
@UdjinM6, given the new logic where these checks always bypass the ignore list, here are my suggestions for renaming ENABLED_CHECKS:
Top recommendations:
ALWAYS_ENABLED_CHECKS- Most descriptive; makes it immediately clear these checks are always shown regardless of the ignore listMANDATORY_CHECKS- Concise and conveys that these must always be reportedENFORCED_CHECKS- Your suggestion; good choice that implies these checks cannot be suppressed
Alternative options:
PRIORITY_CHECKS- Indicates these take priority over ignore rulesCRITICAL_CHECKS- Suggests these are high-importance warningsUNIGNORABLE_CHECKS- Very explicit about the behavior, though slightly awkward
My personal preference would be ALWAYS_ENABLED_CHECKS or MANDATORY_CHECKS as they most clearly communicate the intent. The corresponding regex variable would then be renamed from enabled_regexp to always_enabled_regexp or mandatory_regexp for consistency.
Additionally, you might want to add a comment above the constant explaining: "These checks are always reported, bypassing the IGNORED_WARNINGS filter."
🧠 Learnings used
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
There was a problem hiding this comment.
@coderabbitai any recommendation for "ignored checks" or is it good enough?
There was a problem hiding this comment.
@knst, IGNORED_WARNINGS is fairly clear as-is, but I have a few suggestions for improvement:
Consistency consideration:
There's a minor terminology inconsistency between ENABLED_CHECKS and IGNORED_WARNINGS (checks vs. warnings). For better consistency, you could standardize on one term:
ALWAYS_ENABLED_CHECKS/SUPPRESSED_CHECKS- both use "checks"ALWAYS_ENABLED_WARNINGS/SUPPRESSED_WARNINGS- both use "warnings"
Clarity enhancement options:
If you want to make the behavior more explicit (similar to the reasoning for renaming ENABLED_CHECKS):
SUPPRESSED_WARNINGS- More active voice; clearly indicates these are actively suppressed unless mandatoryFILTERED_WARNINGS- Emphasizes the filtering behaviorALLOWLISTED_SUPPRESSIONS- Very explicit about the allowlist nature
My recommendation:
Keep it as IGNORED_WARNINGS or change to SUPPRESSED_WARNINGS for slightly stronger clarity. The current name is already quite understandable. The more important change is renaming ENABLED_CHECKS since its behavior (bypassing the ignore list) wasn't immediately clear from its name.
If you do rename ENABLED_CHECKS to ALWAYS_ENABLED_CHECKS, you might consider SUPPRESSED_WARNINGS for parallelism (ALWAYS_ENABLED vs. SUPPRESSED makes the distinction very clear). Otherwise, the current name works fine.
🧠 Learnings used
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7018
File: test/lint/lint-github-actions.py:1-9
Timestamp: 2025-12-01T18:13:21.314Z
Learning: In the Dash repository, the file test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types (such as Python files, .ui files, etc.) should not be added to this list.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
|
This pull request has conflicts, please rebase. |
It doesn't seems as it slow down cpp check much, but output is not polluted by these warnings:
Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches. [normalCheckLevelMaxBranches]"
src/batchedlogger.h:0:0: warning: Internal error: Child process crashed with signal 6 [cppcheckError]
They have no use for CI
The previous logic had an intersection of black & white list for exception, and only warnings that got in the white list have been shown. Everything that has not been matched with whitelist should be filtered by blacklist (exceptions), but not only whitelist matches
…ABLED_WARNINGS / SUPPRESSED_WARNINGS
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/lint/lint-cppcheck-dash.py (1)
36-57:SUPPRESSED_WARNINGSset is targeted; TODO can be revisited laterThe suppression list is kept focused on clearly noisy or tool-related diagnostics, and the rename to
SUPPRESSED_WARNINGSmatches its blacklist role. The TODO around the internal-error suppression is fine to leave for now; it can be re-evaluated against future tool versions in a follow-up rather than expanding scope of this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contrib/containers/ci/ci-slim.Dockerfile(1 hunks)src/cachemultimap.h(1 hunks)src/evo/netinfo.h(1 hunks)test/lint/lint-cppcheck-dash.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contrib/containers/ci/ci-slim.Dockerfile
- src/evo/netinfo.h
- src/cachemultimap.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Applied to files:
test/lint/lint-cppcheck-dash.py
🔇 Additional comments (2)
test/lint/lint-cppcheck-dash.py (2)
18-34:ALWAYS_ENABLED_WARNINGSnaming and semantics look correctRenaming and wiring this set into
always_enabled_regexpnow clearly expresses that these patterns always bypass suppression and will still be reported even if they also matchSUPPRESSED_WARNINGS, which matches the intended whitelist behavior.
87-98: Cppcheck command and messaging aligned with new filtering modelAdding
--check-level=exhaustivetogether with the refined ALWAYS/SUPPRESSED pattern sets makes sense for catching more issues while still controlling noise, and the updated advisory message correctly directs contributors toSUPPRESSED_WARNINGSafter the rename.Also applies to: 130-130
| always_enabled_regexp = '|'.join(ALWAYS_ENABLED_WARNINGS) | ||
| suppressed_regexp = '|'.join(SUPPRESSED_WARNINGS) | ||
| files_regexp = '|'.join(re.escape(f) for f in files) |
There was a problem hiding this comment.
Filtering logic matches intended behavior; guard against empty pattern lists
The new condition
if re.search(always_enabled_regexp, line) or not re.search(suppressed_regexp, line):correctly implements: “always show ALWAYS_ENABLED warnings; for everything else, only suppress lines that match SUPPRESSED_WARNINGS”. That fixes the previous whitelist/blacklist intersection issue.
One edge case: if ALWAYS_ENABLED_WARNINGS or SUPPRESSED_WARNINGS is ever made empty, join will produce '', and re.search('', line) matches all lines. That would silently change behavior (e.g. an empty SUPPRESSED_WARNINGS would suddenly hide all non‑ALWAYS lines).
A small defensive tweak keeps current behavior while making the script robust to future edits:
- always_enabled_regexp = '|'.join(ALWAYS_ENABLED_WARNINGS)
- suppressed_regexp = '|'.join(SUPPRESSED_WARNINGS)
+ always_enabled_regexp = '|'.join(ALWAYS_ENABLED_WARNINGS) if ALWAYS_ENABLED_WARNINGS else None
+ suppressed_regexp = '|'.join(SUPPRESSED_WARNINGS) if SUPPRESSED_WARNINGS else None
@@
- if not re.search(files_regexp, line):
- continue
- if re.search(always_enabled_regexp, line) or not re.search(suppressed_regexp, line):
- warnings.append(line)
+ if not re.search(files_regexp, line):
+ continue
+ always_enabled_match = always_enabled_regexp and re.search(always_enabled_regexp, line)
+ suppressed_match = suppressed_regexp and re.search(suppressed_regexp, line)
+ if always_enabled_match or not suppressed_match:
+ warnings.append(line)Also applies to: 119-124
🤖 Prompt for AI Agents
In test/lint/lint-cppcheck-dash.py around lines 75-77 (and similarly at
119-124), the joined regex strings can be empty (''), and re.search('', line)
will match every line; change the construction to guard against empty lists by
replacing an empty join result with a regex that never matches (for example
'(?!)') or by skipping the re.search when the corresponding list is empty, so
that ALWAYS_ENABLED and SUPPRESSED behave correctly even if their lists are
empty.
2 participants
Issue being fixed or feature implemented
The previous logic had an intersection of black & white list for exception, and only warnings that got in the white list have been shown.
Everything that has not been matched with whitelist should be filtered by blacklist (exceptions), but not only whitelist matches
What was done?
evo/mninfo.handcachemultimap.hHow Has This Been Tested?
Run locally
Breaking Changes
N/A
Checklist: