Better static analysis #32
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances code quality by enabling comprehensive static analysis through compiler warnings and the Clang Static Analyzer. It addresses all warnings found by these tools through bug fixes, improved error handling, and clearer code initialization patterns.
Key Changes:
- Enabled strict compiler warnings (
-Wall -Wextra -Wshadow -Wformat=2 -Wnull-dereferenceetc.) in makefiles - Fixed bugs found by static analysis: null pointer dereferences, unused variables, type mismatches, and variable shadowing
- Added Clang Static Analyzer integration to the QA workflow with new
make analyzetarget
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workspace/all/minui/makefile | Added comprehensive compiler warning flags |
| workspace/all/minui/minui.c | Removed unused variable; added null check for opendir |
| workspace/all/minarch/makefile | Enabled compiler warning flags (previously commented out) |
| workspace/all/minarch/minarch.c | Added fall-through comment; converted to designated initializers; fixed variable shadowing; improved naming |
| workspace/all/common/utils.c | Enhanced error handling in getFile and allocFile; removed dead assignment; fixed ftell return type check |
| workspace/all/common/gfx_text.c | Added null pointer check for prev variable |
| workspace/all/common/api.c | Fixed type mismatch: changed uint8_t to int for SDL scancode and button |
| makefile.qa | Added Clang Static Analyzer target with SDL dependencies |
| makefile | Added analyze shortcut target |
| .github/workflows/qa.yml | Added static analysis job to CI pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace cppcheck with clang-tidy throughout the codebase - Create .clang-tidy configuration with focused checks (bugprone, clang-analyzer, performance, portability, cert) - Add workspace/ubuntu platform for CI/linting with complete platform definitions - Track libretro-common/include headers in git (required for CI linting) - Update GitHub Actions workflow to install clang-tidy and SDL2 development libraries - Update makefile.qa with platform-specific configuration (macOS uses real SDL, Linux uses ubuntu platform) - Fix critical bugs found by clang-tidy: * Memory leak in api.c:1209 (unsafe realloc pattern) * Memory leak in collections.c:50 (unsafe realloc pattern) * Uninitialized state_file variables in minarch.c * Null pointer check before strlen in minarch.c:2036 - Remove .cppcheck-suppressions file - Update all documentation to reference clang-tidy 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create workspace/ubuntu platform for CI/linting with complete platform definitions - Add platform-specific function stubs (GetVolume, SetVolume, SetBrightness, etc.) - Restore libretro-common as proper git submodule - GitHub Actions now checks out submodules for complete linting coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 43 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
workspace/all/minui/minui.c:1
- SDL_Rect compound literal includes width and height fields set to 0, which may cause SDL_BlitSurface to render nothing. If the intention is to use the source surface dimensions, the last two fields should be omitted or set to thumb->w and thumb->h.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workspace/all/common/defines.h
Outdated
| #define TRIAD_WHITE 0xff, 0xff, 0xff, 0 | ||
| #define TRIAD_BLACK 0x00, 0x00, 0x00, 0 | ||
| #define TRIAD_LIGHT_GRAY 0x7f, 0x7f, 0x7f, 0 | ||
| #define TRIAD_GRAY 0x99, 0x99, 0x99, 0 | ||
| #define TRIAD_DARK_GRAY 0x26, 0x26, 0x26, 0 | ||
|
|
||
| #define TRIAD_LIGHT_TEXT 0xcc, 0xcc, 0xcc, 0 | ||
| #define TRIAD_DARK_TEXT 0x66, 0x66, 0x66, 0 |
There was a problem hiding this comment.
Adding a fourth component (0) to the TRIAD macros changes their structure from RGB triplets to RGBA quadruplets. This could break existing code that expects exactly 3 values. Verify all usages of these macros accommodate the additional component.
| @@ -1,3 +1,5 @@ | |||
| stella_filter = disabled | |||
| stella_crop_hoverscan = enabled | |||
There was a problem hiding this comment.
Corrected spelling of 'hoverscan' to 'overscan'.
| stella_crop_hoverscan = enabled | |
| stella_crop_overscan = enabled |
nchapman
force-pushed
the
feature/better-static-analysis
branch
5 times, most recently
from
52f2332 to
1b0d276
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 44 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
workspace/all/minarch/minarch.c:1
- The comment indicates this line is never reached because the device powers off. Consider using __builtin_unreachable() or similar to make this explicit to the compiler and static analyzers, or remove the return statement if truly unreachable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| prev += 1; | ||
| line = prev; | ||
| } else { | ||
| line = tmp; |
There was a problem hiding this comment.
The else branch may lead to incorrect text wrapping behavior. When prev is NULL (no previous space found), setting line = tmp skips to the current space position without wrapping, which could result in the first word being lost. Consider handling this case more explicitly or documenting why this behavior is intended.
| line = tmp; | |
| // Insert a newline at the current space to wrap the first word | |
| tmp[0] = '\n'; | |
| line = tmp + 1; |
nchapman
force-pushed
the
feature/better-static-analysis
branch
from
1b0d276 to
5ebdb95
Compare
No description provided.