Repository navigation
fix(darkforces): clear static-analysis findings in the ESP32 port layer - #124
Merged
Merged
Conversation
cppcheck (run on the whole tree, report_pr_changes_only=false) flagged the Dark Forces ESP32 port layer. Fix the ones that are our own code, and suppress the rest, which are bound to the vendored TFE engine's API. Fixed in code: - esp_memory.cpp: default-initialize the internal MemoryBlock / MemoryRegion / CallerStat members (uninitMemberVarNoCtor). - esp_alloc.cpp, esp_audio.cpp: two live old-style C-casts -> C++ reinterpret_cast (dangerousTypeCast). - esp_thread.cpp: const accessors (functionConst), m_done moved to the constructor init list (useInitializationList), and the destructor's waitOnExit() call qualified so it doesn't dispatch virtually (virtualCallInConstructor). Suppressed (suppressions.txt), with rationale: the remaining constParameterPointer / constParameterCallback / constVariablePointer findings and esp_stubs.cpp's two dead-stub casts can't be acted on -- the pointer parameters implement the TFE engine's own (vendored) function/callback signatures, so adding const would diverge from the engine; the allocator's local-pointer consts are cosmetic; and the stub casts are in disabled save-thumbnail code. cppcheck is clean on components/darkforces/src/platform after this. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf
|
✅Static analysis result - no issues found! ✅ |
cppcheck checks the header standalone and parses it as C (it declares a namespace), reporting a false syntaxError at line 6. The file is valid C++ and compiles fine; suppress the false positive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The broad suppressions could conceal future actionable findings throughout the port layer.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates the Dark Forces ESP32 port to address cppcheck findings.
Changes:
- Initializes memory bookkeeping members.
- Modernizes casts and thread implementation details.
- Documents and suppresses unavoidable port-layer findings.
| File | Description |
|---|---|
suppressions.txt |
Adds Dark Forces port suppressions. |
components/darkforces/src/platform/esp_thread.cpp |
Improves constness and initialization. |
components/darkforces/src/platform/esp_memory.cpp |
Adds member defaults. |
components/darkforces/src/platform/esp_audio.cpp |
Replaces a C-style cast. |
components/darkforces/src/platform/esp_alloc.cpp |
Replaces a C-style cast. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ESP-IDF Size Report for 'Esp Box Emu'
FLASH uses app .bin size or json2 flash sum. RAM sums DRAM+IRAM via idf_size. Percentages shown when totals are available. |
…site Addresses review feedback that the file-wide wildcard suppressions in suppressions.txt would also hide future, actionable findings across the whole port layer. - Removed the dir-wide constParameterPointer / constParameterCallback / constVariablePointer / dangerousTypeCast wildcards for components/darkforces/src/platform. - The two allocator local-pointer findings are now fixed in code (const void* / const AllocHeader* in esp_alloc.cpp) rather than suppressed. - The remaining findings -- pointer/callback parameters whose signatures are fixed by the vendored TFE engine -- are suppressed inline at each definition (plain // cppcheck-suppress for isolated ones, and suppress-begin/-end around the cohesive stub blocks in esp_audio, esp_memory and esp_stubs), each with a one-line reason. cppcheck (workflow args) stays clean on the port layer, and new findings elsewhere in it are no longer masked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf
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.
Follow-up to #119 (the ECAD/SDMMC merge), which surfaced the Dark Forces static-analysis findings now that that code is on the branch.
cppcheck runs over the whole tree (
report_pr_changes_only: false) and flagged the Dark Forces ESP32 port layer (components/darkforces/src/platform/). The vendored engine itself (components/darkforces/tfe/) is already suppressed; this PR handles the port glue.Fixed in code (our own code)
MemoryBlock/MemoryRegion/CallerStatmembers (uninitMemberVarNoCtor).reinterpret_cast(dangerousTypeCast).constaccessors (functionConst),m_donemoved to the ctor init list (useInitializationList), and the destructor'swaitOnExit()call qualified so it doesn't dispatch virtually (virtualCallInConstructor).Suppressed (with rationale, in
suppressions.txt)The remaining
constParameterPointer/constParameterCallback/constVariablePointerfindings andesp_stubs.cpp's two dead-stub casts can't be acted on: the pointer parameters implement the TFE engine's own vendored function/callback signatures, so addingconstwould diverge from the engine's declarations and break the override/linkage; the allocator's two local-pointer consts are cosmetic; and the stub casts are in disabled save-thumbnail code.cppcheck(same args as the Static analysis workflow) is clean oncomponents/darkforces/src/platform/after this.🤖 Generated with Claude Code
https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf