Add include/cppcheck linting and extend strict warnings to every build - #81
Merged
Conversation
Introduces two new whole-repo static analysis passes, applies the strict first-party warning set to every build that compiles our own code (not just host), fixes what those turned up, and hardens the release workflows. New lint tooling - script/check-includes.sh + check_includes.py: an include-what-you-use pass over src/, include/, tests/, host_examples/, and examples/, built on clang-include-cleaner. Files with guarded ESP branches are analyzed twice -- a host pass, and an ESP pass with -DESP_PLATFORM against stub headers materialized from script/esp_stubs.py -- so ESP-only code paths are checked without a cross toolchain. A .cpp may still rely on its matching .h's includes. Supports --fix for unused-include removal. - script/cppcheck.sh: whole-program cppcheck (warning, style, unusedFunction) over first-party sources. It sees every caller in a single pass, so it can flag functions that are dead beyond the public API surface -- something per-TU tooling cannot do. - clang-tidy.sh now passes --warnings-as-errors='*' so findings fail CI even if .clang-tidy ever loses its WarningsAsErrors line. - Both new checks run as CI jobs and are required by the merge gate. cppcheck is pinned to 2.21.0 and built from source (cached) because the distro package lags several minor versions and reports differently than the version developers run locally. Warning flags - Extracted the wrapper warning set into opus_wrapper_warning_flags() and applied it to the ESP-IDF component build as well as host. ESP-only code paths (#ifdef ESP_PLATFORM branches, Xtensa sources) previously compiled under strict warnings nowhere, since the host build never sees them. - Added -Wmissing-prototypes/-Wmissing-declarations and -Wold-style-cast to the set, and applied the same flags to test sources and host_examples. - -Werror stays gated behind ENABLE_WERROR (default off), so consumers building these source-distributed files with an arbitrary toolchain get warnings only. CI turns it on for the host sanitizer build and for both PlatformIO example builds, which act as the pinned ESP gate. Fixes for the newly enabled warnings - Replaced C-style casts with static_cast across src/, tests, and the benchmark examples. The end-trim arithmetic now stays in 64-bit rather than narrowing early: the value derives from an untrusted granule position and can exceed SIZE_MAX on 32-bit targets. - Wrapped OPUS_SET_GAIN/OPUS_SET_BITRATE call sites in diagnostic pragmas -- the C-style cast is inside the vendored libopus macro and can't be fixed at the call site. - Made file-local helpers in opus_to_wav static, added its missing <cstdint>, and zero-initialized EncodeResult so the early error-return path returns defined fields. - Annotated the public accessors whose only callers are downstream consumers, which cppcheck's unusedFunction cannot see. API - decode() and conceal_loss() on both decoders are now [[nodiscard]], with a warn_unused_result fallback below C++17. These report errors only through their return code, so ignoring it silently loses failures. - OggOpusDecoder's constructor is now explicit. - ogg_opus_decoder.h uses <cstddef>/<cstdint> instead of the C headers. Release and CI workflows - release-drafter authenticates through a GitHub App token instead of GITHUB_TOKEN plus a deploy key; the workflow's default token is now granted no permissions at all. - publish.yml: added a timeout to every job and re-granted contents: read to the Espressif job, whose job-level permissions block had replaced the workflow default that checkout relies on. - Fixed the PlatformIO cache key: the nested expression inside hashFiles() was passed as a literal string, so the key never varied by example and never matched a real file. - Dependabot PRs now carry dependencies/github-actions labels, and the release-drafter version-resolver labels are force-major/force-minor to avoid colliding with ordinary triage labels.
There was a problem hiding this comment.
Pull request overview
This PR strengthens repo-wide static analysis and warning hygiene by adding new CI lint passes (include checking + cppcheck), extending strict first-party warning flags to more build targets (host, ESP-IDF component, tests, host examples), and applying targeted code fixes/suppressions where newly-enabled diagnostics flag real or vendor-macro issues.
Changes:
- Add whole-repo include-checking (clang-include-cleaner) and cppcheck passes, and wire both into required CI.
- Centralize and expand strict warning flags, applying them to ESP-IDF wrapper sources, tests, and host examples (with
-Werrorgated behindENABLE_WERROR). - Fix/annotate warnings found (casts,
statichelpers, initialization, cppcheck suppressions) and harden release/publish workflows (permissions, timeouts, caching, labeling).
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_raw_packet.cpp | Adds cppcheck suppression to keep an intentional constant-condition assertion. |
| tests/unit/test_chunked.cpp | Wraps vendor macro calls with diagnostic pragmas; fixes narrowing cast in page sequence. |
| tests/tools/measure_zerocopy.cpp | Improves signed/unsigned conversions for iostream reads and percentage math; adds cppcheck suppression. |
| tests/CMakeLists.txt | Introduces a shared strict-warning function for test targets and applies it. |
| src/opus_packet_decoder.cpp | Locally suppresses -Wold-style-cast for vendored libopus control macros. |
| src/opus_header.cpp | Replaces C-style casts with static_cast in header parsing arithmetic. |
| src/ogg_opus_decoder.cpp | Replaces C-style casts, hardens 64-bit arithmetic for trimming, and adds cppcheck suppressions on public accessors. |
| script/esp_stubs.py | Adds ESP-IDF stub headers used by the include-checker’s ESP pass. |
| script/cppcheck.sh | Adds a pinned, whole-program cppcheck pass over first-party sources with suppressions/exclusions. |
| script/clang-tidy.sh | Forces clang-tidy warnings to fail CI via --warnings-as-errors='*'. |
| script/check-includes.sh | Adds a wrapper script to run the include-checker. |
| script/check_includes.py | Adds a clang-include-cleaner based include-what-you-use checker with host+ESP passes and optional fix mode. |
| include/micro_opus/opus_packet_decoder.h | Introduces MICRO_OPUS_NODISCARD and applies it to error-code-returning APIs; adds cppcheck suppression for an inline accessor. |
| include/micro_opus/ogg_opus_decoder.h | Switches to C++ headers, makes ctor explicit, adds MICRO_OPUS_NODISCARD to decode(). |
| host_examples/opus_to_wav/opus_to_wav.cpp | Adds missing <cstdint> and makes file-local helpers static. |
| host_examples/opus_to_wav/CMakeLists.txt | Extends strict warning flags to host example (missing-decls + old-style-cast). |
| examples/encode_benchmark/src/encode_benchmark.cpp | Zero-initializes EncodeResult to avoid undefined fields on early return paths. |
| examples/encode_benchmark/platformio.ini | Forces -DENABLE_WERROR=ON for CI’s pinned PlatformIO ESP build gate. |
| examples/decode_benchmark/src/decode_benchmark.cpp | Replaces C-style cast with static_cast in a FreeRTOS task entrypoint. |
| examples/decode_benchmark/platformio.ini | Forces -DENABLE_WERROR=ON for CI’s pinned PlatformIO ESP build gate. |
| CMakeLists.txt | Applies wrapper strict warnings to ESP-IDF build too and centralizes warning set via helper. |
| cmake/functions.cmake | Adds opus_wrapper_warning_flags() helper to share warning flags across host + ESP builds. |
| .gitignore | Ignores .conventions-version marker file. |
| .github/workflows/release-drafter.yml | Routes release automation via GitHub App token and drops default token permissions. |
| .github/workflows/publish.yml | Adds job timeouts and corrects permissions for checkout in the Espressif publish job. |
| .github/workflows/ci.yml | Adds required include-check + cppcheck jobs; enables -DENABLE_WERROR=ON in sanitizer build; fixes PlatformIO cache key. |
| .github/release-drafter.yml | Renames version-resolver labels to avoid collisions with triage labels. |
| .github/dependabot.yml | Adds default labels to Dependabot PRs. |
Suppressed comments (1)
src/ogg_opus_decoder.cpp:586
- Cppcheck inline suppression syntax is invalid here (missing a space after
cppcheck-suppress). As written, cppcheck may not recognize the suppression and the new cppcheck CI job can fail onfunctionStatic/unusedFunctionwarnings.
// cppcheck-suppress[functionStatic,unusedFunction]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…discard Three fixes from review of the strict-warning rollout: - tests/support/ogg_mux.h: extending -Wconversion -Werror to test sources broke the GCC unit-test build, since the masked byte extractions promote to int/unsigned int before narrowing to uint8_t. GCC flags only three of these sites today (its range analysis clears the rest), so cast every byte extraction rather than just the failing ones -- the file already uses static_cast<uint8_t> elsewhere, and this keeps a stricter GCC from reopening it. Clang does not warn here, which is why it was missed locally. - script/check_includes.py: the checker never traversed tests/, contrary to its documented coverage. Add it as a host-only root, exclude tests/qemu (ESP-IDF firmware, same case as the benchmark examples) and tests/tools (opt-in instrument that needs MICRO_OGG_DEMUXER_DEBUG and debug-only APIs), add tests/support as an include root, and force -xc++ so ogg_mux.h does not interpolate to a C compile-db entry. Six test files are now checked; the one violation this surfaced -- std::move without <utility> in test_raw_packet.cpp -- is fixed here too. - MICRO_OPUS_NODISCARD: MSVC pins __cplusplus to 199711L unless /Zc:__cplusplus is passed, so a C++17 MSVC build silently dropped the attribute. Take the language level from _MSVC_LANG where it is defined.
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.
Introduces two new whole-repo static analysis passes, applies the strict first-party warning set to every build that compiles our own code (not just host), fixes what those turned up, and hardens the release workflows.
New lint tooling
script/check-includes.sh+check_includes.py: an include-what-you-use pass oversrc/,include/,tests/,host_examples/, andexamples/, built on clang-include-cleaner. Files with guarded ESP branches are analyzed twice: a host pass, and an ESP pass with-DESP_PLATFORMagainst stub headers materialized fromscript/esp_stubs.py, so ESP-only code paths are checked without a cross toolchain. A .cpp may still rely on its matching .h's includes. Supports--fixfor unused-include removal.script/cppcheck.sh: whole-program cppcheck (warning, style, unusedFunction) over first-party sources. It sees every caller in a single pass, so it can flag functions that are dead beyond the public API surface.clang-tidy.shnow passes--warnings-as-errors='*'so findings fail CI even if.clang-tidyever loses its WarningsAsErrors line.cppcheckis pinned to 2.21.0 and built from source (cached) because the distro package lags several minor versions and reports differently than the version developers run locally.Warning flags
opus_wrapper_warning_flags()and applied it to the ESP-IDF component build as well as host. ESP-only code paths (#ifdef ESP_PLATFORMbranches, Xtensa sources) previously compiled under strict warnings nowhere, since the host build never sees them.-Wmissing-prototypes/-Wmissing-declarationsand-Wold-style-castto the set, and applied the same flags to test sources andhost_examples.-Werrorstays gated behindENABLE_WERROR(default off), so consumers building these source-distributed files with an arbitrary toolchain get warnings only. CI turns it on for the host sanitizer build and for both PlatformIO example builds, which act as the pinned ESP gate.Fixes for the newly enabled warnings
static_castacrosssrc/, tests, and the benchmark examples. The end-trim arithmetic now stays in 64-bit rather than narrowing early: the value derives from an untrusted granule position and can exceedSIZE_MAXon 32-bit targets.OPUS_SET_GAIN/OPUS_SET_BITRATEcall sites in diagnostic pragmas -- the C-style cast is inside the vendored libopus macro and can't be fixed at the call site.opus_to_wavstatic, added its missing<cstdint>, and zero-initializedEncodeResultso the early error-return path returns defined fields.API
decode()andconceal_loss()on both decoders are now [[nodiscard]], with awarn_unused_resultfallback below C++17. These report errors only through their return code, so ignoring it silently loses failures.OggOpusDecoder's constructor is now explicit.ogg_opus_decoder.huses<cstddef>/<cstdint>instead of the C headers.Release and CI workflows
GITHUB_TOKENplus a deploy key; the workflow's default token is now granted no permissions at all.publish.yml: added a timeout to every job and re-granted contents: read to the Espressif job, whose job-level permissions block had replaced the workflow default that checkout relies on.dependencies/github-actionslabels, and the release-drafter version-resolver labels areforce-major/force-minorto avoid colliding with ordinary triage labels.