From b8f6b42074b9746eb0ce698bc8801794be950066 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 4 Oct 2026 14:37:52 -0500 Subject: [PATCH 1/3] fix(darkforces): clear static-analysis findings in the ESP32 port layer 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 Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf --- .../darkforces/src/platform/esp_alloc.cpp | 2 +- .../darkforces/src/platform/esp_audio.cpp | 2 +- .../darkforces/src/platform/esp_memory.cpp | 22 +++++++++---------- .../darkforces/src/platform/esp_thread.cpp | 11 +++++----- suppressions.txt | 11 ++++++++++ 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/components/darkforces/src/platform/esp_alloc.cpp b/components/darkforces/src/platform/esp_alloc.cpp index ea6c319c..2f8f27a2 100644 --- a/components/darkforces/src/platform/esp_alloc.cpp +++ b/components/darkforces/src/platform/esp_alloc.cpp @@ -104,7 +104,7 @@ namespace { if (!s_active) { return nullptr; } AllocHeader* header = ((AllocHeader*)ptr) - 1; - if ((uint8_t*)header < s_base || header->magic != sessionMagic()) { return nullptr; } + if (reinterpret_cast(header) < s_base || header->magic != sessionMagic()) { return nullptr; } return header; } diff --git a/components/darkforces/src/platform/esp_audio.cpp b/components/darkforces/src/platform/esp_audio.cpp index 33b407e4..37e9f40f 100644 --- a/components/darkforces/src/platform/esp_audio.cpp +++ b/components/darkforces/src/platform/esp_audio.cpp @@ -98,7 +98,7 @@ namespace TFE_Audio if (s_audioThreadCallback) { lock(); - s_audioThreadCallback((f32*)s_sfxBuffer, AUDIO_CALLBACK_BUFFER_SIZE, 1.0f); + s_audioThreadCallback(reinterpret_cast(s_sfxBuffer), AUDIO_CALLBACK_BUFFER_SIZE, 1.0f); unlock(); haveSfx = true; } diff --git a/components/darkforces/src/platform/esp_memory.cpp b/components/darkforces/src/platform/esp_memory.cpp index a328f0ff..93edf09c 100644 --- a/components/darkforces/src/platform/esp_memory.cpp +++ b/components/darkforces/src/platform/esp_memory.cpp @@ -21,20 +21,20 @@ struct MemoryBlock { - MemoryBlock* prev; - MemoryBlock* next; - u32 size; - u32 caller; // return address of the region_alloc() caller (memory attribution) - MemoryRegion* owner; // region the block was allocated from (frees may name a different one) + MemoryBlock* prev = nullptr; + MemoryBlock* next = nullptr; + u32 size = 0; + u32 caller = 0; // return address of the region_alloc() caller (memory attribution) + MemoryRegion* owner = nullptr; // region the block was allocated from (frees may name a different one) }; struct MemoryRegion { - char name[32]; - MemoryBlock* head; - MemoryBlock* tail; - u64 used; - u64 count; + char name[32] = {}; + MemoryBlock* head = nullptr; + MemoryBlock* tail = nullptr; + u64 used = 0; + u64 count = 0; }; namespace TFE_Memory @@ -49,7 +49,7 @@ namespace TFE_Memory static size_t s_poolBytes = 0, s_heapBytes = 0; // Per-caller attribution of region memory (bytes currently allocated). - struct CallerStat { u32 pc; size_t bytes; size_t count; }; + struct CallerStat { u32 pc = 0; size_t bytes = 0; size_t count = 0; }; enum { CALLER_STAT_COUNT = 128 }; static CallerStat* s_callers = nullptr; // CALLER_STAT_COUNT entries (shared memory) static CallerStat* callerStat(u32 pc) diff --git a/components/darkforces/src/platform/esp_thread.cpp b/components/darkforces/src/platform/esp_thread.cpp index 55866773..e2daa17b 100644 --- a/components/darkforces/src/platform/esp_thread.cpp +++ b/components/darkforces/src/platform/esp_thread.cpp @@ -18,8 +18,8 @@ class ThreadEsp : public Thread virtual void resume(); virtual void waitOnExit(void); - ThreadFunc getFunc() { return m_func; } - void* getUserData() { return m_userData; } + ThreadFunc getFunc() const { return m_func; } + void* getUserData() const { return m_userData; } protected: static void threadEntry(void* arg); @@ -28,16 +28,17 @@ class ThreadEsp : public Thread SemaphoreHandle_t m_done = nullptr; }; -ThreadEsp::ThreadEsp(const char* name, ThreadFunc func, void* userData) : Thread(name, func, userData) +ThreadEsp::ThreadEsp(const char* name, ThreadFunc func, void* userData) + : Thread(name, func, userData), m_done(xSemaphoreCreateBinary()) { - m_done = xSemaphoreCreateBinary(); } ThreadEsp::~ThreadEsp() { if (m_isRunning) { - waitOnExit(); + // qualified call: a destructor must not dispatch virtually + ThreadEsp::waitOnExit(); } if (m_done) { diff --git a/suppressions.txt b/suppressions.txt index 25dfd314..fa9d0e26 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -26,3 +26,14 @@ duplicateExpressionTernary:components/genesis/src/genesis.cpp // exclude in static_analysis.yml only covers the base-tree pass; the PR // tree is checked under pr_tree/, so suppress by relative path here.) *:components/darkforces/tfe/* +// The Dark Forces ESP32 port layer (components/darkforces/src/platform) bridges +// to the vendored TFE engine. These remaining style findings can't be acted on +// in our code: the pointer parameters implement TFE's engine/callback API, so +// adding const would diverge from the engine's own (vendored) declarations; the +// allocator's two local-pointer consts are cosmetic; and esp_stubs.cpp's two +// C-casts are in disabled save-thumbnail stubs. The real findings (uninitialized +// members, the two live C-casts, and the thread class) are fixed in code. +constParameterPointer:components/darkforces/src/platform/* +constParameterCallback:components/darkforces/src/platform/* +constVariablePointer:components/darkforces/src/platform/* +dangerousTypeCast:components/darkforces/src/platform/esp_stubs.cpp From 9dab9e7ed263e3a7b89b4eb2d2d3e3068fc0b25b Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 4 Oct 2026 14:52:41 -0500 Subject: [PATCH 2/3] ci: suppress cppcheck's bogus syntaxError on esp_platform.h 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 Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf --- suppressions.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/suppressions.txt b/suppressions.txt index fa9d0e26..d1906f8a 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -37,3 +37,7 @@ constParameterPointer:components/darkforces/src/platform/* constParameterCallback:components/darkforces/src/platform/* constVariablePointer:components/darkforces/src/platform/* dangerousTypeCast:components/darkforces/src/platform/esp_stubs.cpp +// cppcheck parses this C++ header (it declares a namespace) as C when it +// checks it standalone and reports a bogus syntaxError; the file is valid C++ +// and compiles fine. Suppress that false positive. +syntaxError:components/darkforces/src/platform/esp_platform.h From 121b2040c29faa9b09c49caef064040e61566283 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 4 Oct 2026 15:37:27 -0500 Subject: [PATCH 3/3] fix(darkforces): scope the cppcheck suppressions to each constrained 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 Claude-Session: https://claude.ai/code/session_014M3Ezo5uTqn1QXTWTC6fAf --- components/darkforces/src/platform/esp_alloc.cpp | 6 ++++-- components/darkforces/src/platform/esp_audio.cpp | 7 ++++++- components/darkforces/src/platform/esp_memory.cpp | 4 ++++ components/darkforces/src/platform/esp_render.cpp | 6 ++++++ components/darkforces/src/platform/esp_stubs.cpp | 9 +++++++++ suppressions.txt | 15 +++++---------- 6 files changed, 34 insertions(+), 13 deletions(-) diff --git a/components/darkforces/src/platform/esp_alloc.cpp b/components/darkforces/src/platform/esp_alloc.cpp index 2f8f27a2..ec58fae9 100644 --- a/components/darkforces/src/platform/esp_alloc.cpp +++ b/components/darkforces/src/platform/esp_alloc.cpp @@ -115,7 +115,7 @@ namespace TFE_Memory::PoolLock lock; // Keep malloc's 8 byte alignment in front of the header. const size_t total = size + sizeof(AllocHeader) + 8; - void* mem = poolAllocLocked(total); + const void* mem = poolAllocLocked(total); if (mem) { uintptr_t user = ((uintptr_t)mem + sizeof(AllocHeader) + 7) & ~(uintptr_t)7; @@ -163,7 +163,7 @@ namespace size_t oldSize = 0; { TFE_Memory::PoolLock lock; - AllocHeader* header = liveHeader(ptr); + const AllocHeader* header = liveHeader(ptr); if (!header) { printf("[DarkForces] ERROR: realloc of a stale block pointer %p\n", ptr); @@ -189,6 +189,8 @@ namespace size_t printed; }; + // censusVisit matches the pool walker's callback signature; mem can't take const. + // cppcheck-suppress constParameterCallback void censusVisit(void* mem, size_t size, int used, void* user) { if (!used) { return; } diff --git a/components/darkforces/src/platform/esp_audio.cpp b/components/darkforces/src/platform/esp_audio.cpp index 37e9f40f..fe97e588 100644 --- a/components/darkforces/src/platform/esp_audio.cpp +++ b/components/darkforces/src/platform/esp_audio.cpp @@ -65,6 +65,8 @@ namespace TFE_Audio static AudioThreadCallback s_audioThreadCallback = nullptr; static std::unique_ptr s_task; + // audioTask matches espp::Task's callback signature; its params can't take const. + // cppcheck-suppress constParameterCallback static bool audioTask(std::mutex& m, std::condition_variable& cv, bool& task_notified) { TFE_Memory::DfAllocScope allocScope; @@ -271,7 +273,9 @@ namespace TFE_Audio if (s_lock) { xSemaphoreGiveRecursive(s_lock); } } - // Sound sources are not used by Dark Forces (everything goes through iMuse). + // Sound sources are not used by Dark Forces (everything goes through iMuse). These are + // stub implementations of the TFE_Audio API; the engine's declarations are non-const. + // cppcheck-suppress-begin constParameterPointer bool playOneShot(SoundType type, f32 volume, const SoundBuffer* buffer, bool looping, SoundFinishedCallback finishedCallback, void* cbUserData, s32 cbArg) { return false; } SoundSource* createSoundSource(SoundType type, f32 volume, const SoundBuffer* buffer, SoundFinishedCallback callback, void* userData) { return nullptr; } s32 getSourceSlot(SoundSource* source) { return -1; } @@ -283,6 +287,7 @@ namespace TFE_Audio void setSourceBuffer(SoundSource* source, const SoundBuffer* buffer) {} bool isSourcePlaying(SoundSource* source) { return false; } f32 getSourceVolume(SoundSource* source) { return 0.0f; } + // cppcheck-suppress-end constParameterPointer } void espbox_shared_audio(bool alloc) diff --git a/components/darkforces/src/platform/esp_memory.cpp b/components/darkforces/src/platform/esp_memory.cpp index 93edf09c..6d5c8715 100644 --- a/components/darkforces/src/platform/esp_memory.cpp +++ b/components/darkforces/src/platform/esp_memory.cpp @@ -297,6 +297,9 @@ namespace TFE_Memory return false; } + // The region_* functions implement the TFE_Memory API; the engine declares the + // MemoryRegion* / FileStream* parameters non-const, so they can't take const here. + // cppcheck-suppress-begin constParameterPointer size_t region_getMemoryUsed(MemoryRegion* region) { return region ? region->used : 0; @@ -332,6 +335,7 @@ namespace TFE_Memory { return nullptr; } + // cppcheck-suppress-end constParameterPointer void region_test() { diff --git a/components/darkforces/src/platform/esp_render.cpp b/components/darkforces/src/platform/esp_render.cpp index 359c7890..c9ab6109 100644 --- a/components/darkforces/src/platform/esp_render.cpp +++ b/components/darkforces/src/platform/esp_render.cpp @@ -103,6 +103,8 @@ namespace TFE_RenderBackend return s_frameCount; } + // captureScreenToMemory implements the TFE screenshot API (mem is engine-declared non-const). + // cppcheck-suppress constParameterPointer void captureScreenToMemory(u32* mem) {} void queueScreenshot(const char* screenshotPath) {} void startGifRecording(const char* path) {} @@ -160,6 +162,8 @@ namespace TFE_RenderBackend } void bindVirtualDisplay() {} + // clearVirtualDisplay implements the TFE render-backend API (color is engine-declared non-const). + // cppcheck-suppress constParameterPointer void clearVirtualDisplay(f32* color, bool clearColor) {} void copyToVirtualDisplay(RenderTargetHandle src) {} void copyBackbufferToRenderTarget(RenderTargetHandle dst) {} @@ -224,6 +228,8 @@ namespace TFE_RenderBackend TextureGpu* createTexture(u32 width, u32 height, TexFormat format) { return nullptr; } TextureGpu* createTextureArray(u32 width, u32 height, u32 layers, u32 channels) { return nullptr; } TextureGpu* createTexture(u32 width, u32 height, const u32* data, MagFilter magFilter) { return nullptr; } + // freeTexture implements the TFE render-backend API (texture is engine-declared non-const). + // cppcheck-suppress constParameterPointer void freeTexture(TextureGpu* texture) {} void getTextureDim(TextureGpu* texture, u32* width, u32* height) { *width = 0; *height = 0; } void* getGpuPtr(const TextureGpu* texture) { return nullptr; } diff --git a/components/darkforces/src/platform/esp_stubs.cpp b/components/darkforces/src/platform/esp_stubs.cpp index 392d67a8..caa17267 100644 --- a/components/darkforces/src/platform/esp_stubs.cpp +++ b/components/darkforces/src/platform/esp_stubs.cpp @@ -34,6 +34,10 @@ namespace TFE_DarkForces extern JBool s_palModified; } +// Everything below implements The Force Engine's platform/stub API. The engine's +// (vendored) declarations take non-const pointers, so these definitions can't add +// const to their pointer parameters without diverging from the engine. +// cppcheck-suppress-begin constParameterPointer ///////////////////////////////////////////// // Front end UI ///////////////////////////////////////////// @@ -161,6 +165,9 @@ namespace TFE_Image void free(Image* image) {} void freeAll() {} + // These two are dead (save thumbnails are disabled) and deliberately alias the + // caller's const pixel buffer; the TFE Image signatures are non-const. + // cppcheck-suppress-begin dangerousTypeCast size_t writeImageToMemory(u8*& output, u32 width, u32 height, const u32* pixelData) { output = (u8*)pixelData; @@ -173,6 +180,7 @@ namespace TFE_Image output->height = TFE_SaveSystem::SAVE_IMAGE_HEIGHT; output->data = (u32*)pixelData; } + // cppcheck-suppress-end dangerousTypeCast void writeImage(const char* path, u32 width, u32 height, u32* pixelData) { @@ -343,3 +351,4 @@ namespace TFE_A11Y bool cutsceneCaptionsEnabled() { return false; } bool gameplayCaptionsEnabled() { return false; } } +// cppcheck-suppress-end constParameterPointer diff --git a/suppressions.txt b/suppressions.txt index d1906f8a..3455d2e3 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -27,16 +27,11 @@ duplicateExpressionTernary:components/genesis/src/genesis.cpp // tree is checked under pr_tree/, so suppress by relative path here.) *:components/darkforces/tfe/* // The Dark Forces ESP32 port layer (components/darkforces/src/platform) bridges -// to the vendored TFE engine. These remaining style findings can't be acted on -// in our code: the pointer parameters implement TFE's engine/callback API, so -// adding const would diverge from the engine's own (vendored) declarations; the -// allocator's two local-pointer consts are cosmetic; and esp_stubs.cpp's two -// C-casts are in disabled save-thumbnail stubs. The real findings (uninitialized -// members, the two live C-casts, and the thread class) are fixed in code. -constParameterPointer:components/darkforces/src/platform/* -constParameterCallback:components/darkforces/src/platform/* -constVariablePointer:components/darkforces/src/platform/* -dangerousTypeCast:components/darkforces/src/platform/esp_stubs.cpp +// to the vendored TFE engine. The findings that can't be acted on -- pointer +// parameters that implement TFE's engine/callback signatures -- are suppressed +// inline at each constrained definition (see the cppcheck-suppress comments in +// the .cpp files); the actionable findings are fixed in code. Nothing for the +// port layer is suppressed file-wide here. // cppcheck parses this C++ header (it declares a namespace) as C when it // checks it standalone and reports a bogus syntaxError; the file is valid C++ // and compiles fine. Suppress that false positive.