[RNE Rewrite] chore: add clang-tidy static checks#1286
Draft
msluszniak wants to merge 7 commits into
Draft
Conversation
msluszniak
added
improvement
12 tasks
msluszniak
added a commit
that referenced
this pull request
Jun 26, 2026
## Description Sets up a committed [clangd](https://clangd.llvm.org/) config for the core package's C/C++ sources so the rewrite gets code intelligence and a shared, strict warning set, and enforces that set at commit time. First step of #1271; clang-tidy CI is a follow-up (#1286). - `compile_flags.txt` — include roots / `-std=c++20` / defines, with third-party headers (ExecuTorch/torch/JSI) as `-isystem` so their warnings don't pollute our diagnostics. Relative paths anchored to the package dir, so it is portable with no per-machine generation. - `.clangd` — strict warning set layered on top (incl. `-Wconversion`/`-Wsign-conversion`), with clangd's include cleaner disabled (ExecuTorch umbrella headers misreport includes). - `scripts/check-cpp-warnings.sh` + `lefthook.yml` — a `pre-commit` command that compiles staged `cpp/` sources with the same warning set and aborts the commit on any warning, keeping the editor and the commit gate in sync. It skips gracefully (never blocks) when no compiler or the provisioned headers are available; bypass with `git commit --no-verify`. - `.gitignore` — track the shared config while keeping generated `compile_commands.json` local. - `CONTRIBUTING.md` — prerequisites (provisioned `third-party/include` + `yarn install`), editor setup, and the pre-commit behavior. ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [ ] iOS - [ ] Android ### Testing instructions 1. Provision `packages/react-native-executorch/third-party/include` and run `yarn install`. 2. Open a file under `packages/react-native-executorch/cpp` in an editor with the clangd extension (MS C/C++ IntelliSense disabled); confirm the `<executorch/...>`/`<jsi/jsi.h>` includes resolve and the current sources are clean. 3. Confirm the strict flags fire by introducing a deliberate violation, e.g. in `cpp/core/dtype.cpp`: ```cpp int rne_probe(float f) { int casted = (int)f; // expect -Wold-style-cast + -Wconversion int unused = 7; // expect -Wunused-variable return casted; } ``` clangd should flag these on our code (third-party headers stay quiet thanks to `-isystem`). 4. `git add` that change and `git commit` — the pre-commit hook aborts with the warning printed. Verified end-to-end via lefthook: warning → `cpp-warnings ❌` (exit 1); clean → `✔️` (exit 0). Revert afterwards. ### Related issues #1271 ### Checklist - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings
msluszniak
force-pushed
the
@ms/clang-tidy-ci
branch
from
3d3b42b to
6413497
Compare
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.
msluszniak
force-pushed
the
@ms/clang-tidy-ci
branch
from
6413497 to
352bf27
Compare
Turn on the cppcoreguidelines-* group and fix what it surfaced: - dtype.h: make DType a scoped enum (use-enum-class) - tensor.cpp: initialize dtype_/shape_/numel_ in the member initializer list (prefer-member-initializer) - box_ops/image_ops: value-initialize the parse-result locals (init-variables) - tensor: NOLINT the owning unique_ptr<uint8_t[]> byte buffer (avoid-c-arrays) Exclude the checks incompatible with a native tensor/buffer library: the pro-bounds-* family and pro-type-reinterpret-cast (raw buffer pointer math, hot-loop indexing, type-erased casts) plus the opinionated avoid-magic-numbers.
Turn on the google-* group and fix what it surfaced: - model.h: mark the single-argument ModelHostObject ctor explicit (google-explicit-constructor; matches TokenizerHostObject) - RnExecutorch.cpp: replace `using namespace facebook` with the `namespace jsi = facebook::jsi` alias used elsewhere (google-build-using-namespace) - tensor.cpp: static_cast<size_t>(1) instead of the C-style size_t(1) (google-readability-casting)
Turn on the llvm-* group and fix what it surfaced: - declare the JSI function-name locals as `const auto *name` (llvm-qualified-auto) Exclude llvm-header-guard (the project uses #pragma once) and llvm-prefer-static-over-anonymous-namespace (anonymous namespaces are a valid idiom here and also hold the helpers' shared types/constants). The separate llvmlibc-* module is not pulled in by llvm-*.
Turn on the misc-* group. The useful misc checks (definitions-in-headers, redundant-expression, unused-using-decls, …) stay enabled; the noisy/inapplicable ones are excluded: - misc-const-correctness: applied once as a cleanup (const on ~87 immutable locals, west-const to match the codebase), but excluded from the gate — it is too aggressive to enforce (it flags RAII lock guards as const-able, which we keep non-const) - misc-include-cleaner: ExecuTorch umbrella headers make it misreport includes - misc-multiple-inheritance: the JSI HostObject + enable_shared_from_this pattern - misc-non-private-member-variables-in-classes: HostObjects are deliberate public data carriers read across extensions
Turn on the modernize-* group and fix what it surfaced:
- use-auto on cast-initialized locals; use-emplace over push_back(T(...));
pass-by-value + std::move for the TokenizerHostObject sink ctor param;
designated initializer for FitBox; std::cmp_* for a signed/unsigned compare;
transparent std::multiplies<>
- extend the avoid-c-arrays NOLINTs to also cover modernize-avoid-c-arrays
(alias) on the owning unique_ptr<uint8_t[]> buffer
Exclude modernize-use-trailing-return-type (the codebase uses traditional
return types) and modernize-return-braced-init-list (`return {…}` hides the
JSI return type).
Turn on the readability-* group (completing the check set). Fix isolate-declaration (split the multi-declarations onto separate lines). Exclude the opinionated checks: - readability-identifier-length — short names (i, rt, h, w) are idiomatic here - readability-math-missing-parentheses — flags standard array-offset arithmetic - readability-function-cognitive-complexity — the JSI get() dispatchers and the image/tensor kernels are inherently above the threshold - readability-uppercase-literal-suffix — the codebase uses lowercase (1.0f) - readability-magic-numbers — opinionated (matches the cppcoreguidelines exclusion)
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
1 participant
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.
Description
Adds clang-tidy static checks for the core package's C/C++ sources. Second step of #1271 (clangd setup landed in #1285). Rebased onto
rne-rewritenow that #1285 is merged, so this is clang-tidy-only..clang-tidy— a deliberately conservative, high-signal set (bugprone-*,performance-*,clang-analyzer-*, minus the noisyeasily-swappable-parameters/enum-size), analyzing only our own headers. Green on current sources; the set is meant to grow over time, group by group.scripts/clang-tidy.sh+ thelint:cpppackage script — run clang-tidy overcpp/usingcompile_flags.txt(the same database clangd uses), failing on any finding..github/workflows/clang-tidy.yml— CI gate, dormant behind theENABLE_CLANG_TIDYrepo variable. It provisions headers through the on-demand download flow (download-libs.js, [RNE Rewrite] feat: on-demand native lib download and backend splitting #1283) rather than a stub, since clang-tidy needs the ExecuTorch headers to parse the sources.cpp/core/dtype.cpp—NOLINTthe intentional identical-sizeswitchbranches.Depends on #1283 for
download-libs.js; enable the workflow once that has merged and a release carryingheaders.tar.gzexists. The provisioning step setsRNET_HEADERS_ONLY=1(clang-tidy needs only headers) — see PR thread re: honoring that flag indownload-libs.js.Verified locally (Homebrew LLVM clang-tidy):
lint:cppis green on all sources and exits non-zero on an injectedbugprone-integer-division.Introduces a breaking change?
Type of change
Tested on
Testing instructions
packages/react-native-executorch/third-party/includeand runyarn install.CLANG_TIDY=$(brew --prefix llvm)/bin/clang-tidy yarn workspace react-native-executorch lint:cpp— expect 0 findings.cpp/file, e.g.double r = a / b;(ints) forbugprone-integer-division, and re-run — expect a non-zero exit. Revert afterwards.Related issues
#1271
Checklist