Skip to content

Commit 87a2237

Browse files
msluszniakmsluszniak
committed
[RNE Rewrite] chore: add clang-tidy static checks
A conservative, high-signal clang-tidy config plus a runner and a (dormant) CI workflow. - .clang-tidy: bugprone-* / performance-* / clang-analyzer-* (minus the noisy easily-swappable-parameters and enum-size), analyzing only our own headers. - scripts/clang-tidy.sh + `lint:cpp`: run clang-tidy over cpp/ using compile_flags.txt, failing on any finding. - .github/workflows/clang-tidy.yml: gated behind ENABLE_CLANG_TIDY; provisions headers via the on-demand download flow (download-libs.js, #1283) since clang-tidy needs the ExecuTorch headers to parse the sources. - dtype.cpp: NOLINT the intentional identical-size switch branches.
1 parent 95acdba commit 87a2237

6 files changed

Lines changed: 131 additions & 0 deletions

File tree

‎.github/workflows/clang-tidy.yml‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
name: clang-tidy
2+
3+
on:
4+
pull_request:
5+
branches:
6+
- main
7+
- rne-rewrite
8+
paths:
9+
- 'packages/react-native-executorch/cpp/**'
10+
- 'packages/react-native-executorch/.clang-tidy'
11+
- 'packages/react-native-executorch/compile_flags.txt'
12+
- 'packages/react-native-executorch/scripts/clang-tidy.sh'
13+
- '.github/workflows/clang-tidy.yml'
14+
workflow_dispatch:
15+
16+
jobs:
17+
clang-tidy:
18+
runs-on: ubuntu-latest
19+
# Dormant until the on-demand header artifact is published (#1283): clang-tidy
20+
# must parse the sources, which needs the ExecuTorch/torch headers that are not
21+
# committed. Enable by setting the repo variable ENABLE_CLANG_TIDY=true once a
22+
# release carrying headers.tar.gz exists for the package version.
23+
if: ${{ vars.ENABLE_CLANG_TIDY == 'true' }}
24+
steps:
25+
- name: Checkout
26+
uses: actions/checkout@v6
27+
28+
- name: Setup
29+
uses: ./.github/actions/setup
30+
31+
- name: Install clang-tidy
32+
run: |
33+
sudo apt-get update
34+
sudo apt-get install -y clang-tidy
35+
clang-tidy --version
36+
37+
- name: Provision ExecuTorch headers
38+
# Reuse the on-demand download flow (#1283). clang-tidy only needs headers
39+
# (it syntax-checks, no linking), so RNET_HEADERS_ONLY fetches just
40+
# headers.tar.gz; RNET_TARGET avoids host platform auto-detection.
41+
working-directory: packages/react-native-executorch
42+
env:
43+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
44+
RNET_HEADERS_ONLY: '1'
45+
RNET_TARGET: android-arm64-v8a
46+
run: node scripts/download-libs.js
47+
48+
- name: Run clang-tidy
49+
run: yarn workspace react-native-executorch lint:cpp

‎CONTRIBUTING.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,20 @@ don't conflict. clangd discovers `compile_flags.txt`/`.clangd` automatically fro
7979
open file's directory. If you produce a `compile_commands.json` from a native build,
8080
clangd will prefer it — it's gitignored and takes precedence over `compile_flags.txt`.
8181

82+
## clang-tidy
83+
84+
`packages/react-native-executorch/.clang-tidy` adds a conservative, high-signal set of
85+
static checks (bugprone / performance / clang-analyzer) on top of the same database. The
86+
clangd extension surfaces these inline; you can also run them from the command line:
87+
88+
```
89+
yarn workspace react-native-executorch lint:cpp # all C++ sources
90+
CLANG_TIDY=$(brew --prefix llvm)/bin/clang-tidy yarn workspace react-native-executorch lint:cpp
91+
```
92+
93+
It needs the same provisioned headers as clangd. Suppress an intentional, reviewed finding
94+
with a commented `// NOLINTNEXTLINE(check-name)` rather than loosening the shared config.
95+
8296
# Creating a Pull Request
8397

8498
Before writing any code reach out to us to make sure no one is currently working on it, you can always open an issue first.

‎packages/react-native-executorch/.clang-tidy‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# clang-tidy configuration for the react-native-executorch C/C++ sources.
2+
#
3+
# This is the first, deliberately conservative step toward a clang-tidy CI gate
4+
# (issue #1271): a tight, high-signal set focused on correctness and performance
5+
# bugs rather than opinionated style. It is expected to stay green on our own
6+
# code; grow the check list over time.
7+
#
8+
# clang-tidy reads the include flags / standard / defines from compile_flags.txt
9+
# (the same database clangd uses), so it needs the same prerequisites: a provisioned
10+
# third-party/include and `yarn install`. The clangd extension also surfaces these
11+
# checks inline in the editor.
12+
Checks: >
13+
-*,
14+
bugprone-*,
15+
performance-*,
16+
clang-analyzer-*,
17+
-bugprone-easily-swappable-parameters,
18+
-performance-enum-size
19+
20+
# Restrict analysis to this package's own cpp/ tree. Anchored on the full package
21+
# path so it can never match an unrelated directory that merely contains "cpp"
22+
# (e.g. third-party/.../abseil-cpp/, or another package's cpp/). Third-party headers
23+
# are also -isystem in compile_flags.txt, so clang-tidy does not analyze them anyway.
24+
HeaderFilterRegex: 'packages/react-native-executorch/cpp/.*\.(h|hpp)$'
25+
26+
# Treat findings as warnings (CI decides failure via --warnings-as-errors).
27+
WarningsAsErrors: ''

‎packages/react-native-executorch/cpp/core/dtype.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ size_t elementSize(DType dtype) {
5454
switch (dtype) {
5555
case DType::uint8:
5656
return 1;
57+
// int32 and float32 are both 4 bytes; the identical branches are intentional.
58+
// NOLINTNEXTLINE(bugprone-branch-clone)
5759
case DType::int32:
5860
return 4;
5961
case DType::float32:

‎packages/react-native-executorch/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
"prepare": "bob build",
4646
"typecheck": "tsc --noEmit",
4747
"lint": "eslint \"**/*.{js,ts,tsx}\"",
48+
"lint:cpp": "./scripts/clang-tidy.sh",
4849
"prepack": "cp ../../README.md ./README.md",
4950
"postpack": "rm ./README.md",
5051
"postinstall": "node scripts/download-libs.js && yarn run -T patch-package --patch-dir ../../patches"

‎packages/react-native-executorch/scripts/clang-tidy.sh‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#!/usr/bin/env bash
2+
#
3+
# Runs clang-tidy over the package's own C/C++ sources, using the checks in
4+
# .clang-tidy and the include flags from compile_flags.txt (the same database
5+
# clangd uses). Any finding fails the run (--warnings-as-errors).
6+
#
7+
# Usage:
8+
# scripts/clang-tidy.sh [file ...] # defaults to every *.cpp under cpp/
9+
#
10+
# Prerequisites (same as a native build): a provisioned third-party/include and
11+
# `yarn install` at the repo root, so the third-party / JSI includes resolve.
12+
# Override the binary with CLANG_TIDY=/path/to/clang-tidy (e.g. Homebrew LLVM).
13+
set -euo pipefail
14+
15+
cd "$(dirname "$0")/.."
16+
17+
CLANG_TIDY="${CLANG_TIDY:-clang-tidy}"
18+
19+
if ! command -v "$CLANG_TIDY" >/dev/null 2>&1; then
20+
echo "error: '$CLANG_TIDY' not found. Install LLVM (e.g. 'brew install llvm' or" \
21+
"'apt-get install clang-tidy') or set CLANG_TIDY to its path." >&2
22+
exit 127
23+
fi
24+
25+
if [ "$#" -gt 0 ]; then
26+
files=("$@")
27+
else
28+
files=()
29+
while IFS= read -r f; do files+=("$f"); done < <(find cpp -name '*.cpp' | sort)
30+
fi
31+
32+
if [ "${#files[@]}" -eq 0 ]; then
33+
echo "No C++ sources to check."
34+
exit 0
35+
fi
36+
37+
echo "Running clang-tidy on ${#files[@]} file(s)…"
38+
exec "$CLANG_TIDY" --warnings-as-errors='*' "${files[@]}"

0 commit comments

Comments
 (0)