Run clang-tidy on pull-request changed lines#4982
Conversation
The repository already ships a strict .clang-tidy (bugprone-*, cert-*, clang-analyzer-*, ... with WarningsAsErrors) but no workflow runs it, so the bug class it catches is currently caught only by a reviewer reading the diff. Add a pull_request workflow that runs clang-tidy-14 on the lines a PR changes (clang-tidy-diff.py driven from the base SHA, the same diff-scoping approach as the git-clang-format gate in coding_guidelines.yml), so a PR is only flagged for issues it introduces on the lines it touches. Like that gate it needs only contents:read and computes the diff from the pull_request base SHA, so it behaves identically for in-repo and forked pull requests. compile_commands.json is produced from the default linux iwasm build (no LLVM required), covering the highest-traffic loader / interpreter / common sources.
matthargett
requested review from
TianlongLiang,
lum1n0us and
yamt
as code owners
lum1n0us
left a comment
lum1n0us
left a comment
There was a problem hiding this comment.
Before turning this workflow into an enforced check, should we run a full clang-tidy scan on the current codebase and clean up any existing issues first? Otherwise, contributors may end up having to deal with pre-existing diagnostics while making unrelated changes in their PRs.
| -G Ninja \ | ||
| -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON | ||
| cmake --build build --target iwasm --parallel |
There was a problem hiding this comment.
For compile_commands.json, there is no need to cmake --build build
| with: | ||
| # full history so the pull_request base SHA is available for the diff | ||
| fetch-depth: 0 | ||
|
|
There was a problem hiding this comment.
Maybe add a quick-exit path for those PRs only affect documents, test cases, CIs to save action minutes?
| cmake -S product-mini/platforms/linux -B build \ | ||
| -G Ninja \ | ||
| -DCMAKE_BUILD_TYPE=Debug \ | ||
| -DCMAKE_EXPORT_COMPILE_COMMANDS=ON |
There was a problem hiding this comment.
Generating compile_commands.json from the default Linux configuration does cover the highest-traffic sources, but it does not guarantee coverage of every file or every changed line in a PR. As a result, this workflow may miss some modified code paths or analyze files without the correct compilation context
| -p1 -path build \ | ||
| -clang-tidy-binary "$TIDY" \ | ||
| -j "$(nproc)" 2>&1 \ | ||
| | tee clang-tidy.log ; then |
There was a problem hiding this comment.
When clang-tidy is asked to analyze a changed file that is not present in compile_commands.json , it may fall back to analyzing the file directly without the proper build context. That can produce false failures due to missing macro definitions, wrong conditional-compilation branches, missing include paths, or even diagnostics on unchanged lines in the modified file.
| with: | ||
| # full history so the pull_request base SHA is available for the diff | ||
| fetch-depth: 0 | ||
|
|
There was a problem hiding this comment.
It may be worth adding a quick-exit path for PRs that only change documentation, test data, or other non-source files. In those cases, the workflow could skip the clang-tidy step entirely and save GitHub Actions minutes.
2 participants
What
Run the repository's existing
.clang-tidyon pull requests — but only on the lines the PR changes.The root
.clang-tidyis already a maintained, strict config (bugprone-*,cert-*,clang-analyzer-*, ... withWarningsAsErrors), but no workflow runs it today, so the null-deref / sign-conversion / use-after-move class of bug it catches is currently caught only by a reviewer reading the diff.How
Mirrors the diff-scoping already used by the
git-clang-format-14gate incoding_guidelines.yml:permissions: contents: read,fetch-depth: 0, diff computed from${{ github.event.pull_request.base.sha }}..HEAD.clang-tidy-diff.py(pinned to the llvm-14 release) turns the changed-line ranges into-line-filter, so a PR is only flagged for issues it introduces on the lines it touches — pre-existing issues on untouched lines stay silent.compile_commands.jsoncomes from the default linuxiwasmbuild (interp + AOT runtime, no LLVM required), covering the highest-traffic loader / interpreter / common sources.Fork / upstream safety
Same properties as
coding_guidelines.yml: onlycontents: read, diff from the base SHA — so it behaves identically for in-repo and forked pull requests.Notes
ci/coding_guidelines_check.py.pull_requestonly: the gate diffs against the PR base, so a manual (workflow_dispatch) run would have no meaningful base.Verification
Verified on a pull request in a fork with clang-tidy-14: a violation on a changed line fails the job; a clean change passes; a pre-existing violation on an untouched line does not fire (diff-scoping). A PR that touches no C/C++ lines is a clean no-op.