[RNE Rewrite] chore: add clang-tidy static checks#1286
Merged
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
2 times, most recently
from
6413497 to
352bf27
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.
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)
These CV/math functions landed via the instance-segmentation pipeline (#1289) after the per-group passes, so the rebase onto rne-rewrite surfaced findings the groups had not yet seen. Apply the same idioms used elsewhere: - const auto *name for the JSI function-name locals (llvm-qualified-auto) - value-initialize the parse-result locals boxFormat / cvType (init-variables) - use auto for the cast-initialized x1/y1/x2/y2 and thresholdVal (use-auto)
msluszniak
force-pushed
the
@ms/clang-tidy-ci
branch
from
7462a5e to
aa0d60c
Compare
3 tasks
download-libs.js resolves the release tag from the package version
(v${PACKAGE_VERSION} == v0.0.0), but the placeholder 0.0.0 has no release;
headers.tar.gz currently lives only on v0.0.0-rewrite-libs-test. Pin
RNET_BASE_URL to that pre-release so the gate can run. Tracked in #1291 —
drop the override once a real versioned release exists.
barhanc
reviewed
Jun 29, 2026
barhanc
left a comment
barhanc
left a comment
Contributor
There was a problem hiding this comment.
One tiny thing I noticed regarding .clangd is that it flags #pragma once in header files. Maybe we could also fix that in this PR.
Member
Author
Ok, here I explicitly turned of suggestion about LLVM style header guards so it's just for clangd. |
Per review: every NOLINT carries an inline rationale after a colon. - tensor.cpp / tensor.h: ": owning runtime-sized byte buffer" (em dash -> colon on the ctor line; add the missing rationale on the data_ member) - dtype.cpp: fold the separate explanatory comment onto the NOLINTNEXTLINE line
compile_flags.txt forces -xc++, so clangd opens headers as main-file translation units and emits -Wpragma-once-outside-header for their guards. Add -Wno-pragma-once-outside-header to .clangd to silence that editor-only false positive; the rest of the strict warning set is unaffected, and clang-tidy never sees it (it only parses headers via #include).
barhanc
approved these changes
Jun 29, 2026
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
2 participants
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 analysis for the core package's C/C++ sources. Second step of #1271 (clangd setup landed in #1285). Rebased onto
rne-rewrite, so this is clang-tidy-only..clang-tidy— a high-signal check set enabled group by group (one commit each):bugprone-*,performance-*,clang-analyzer-*,cppcoreguidelines-*,google-*,llvm-*,misc-*,modernize-*,readability-*. Each group's narrow exclusion list (raw-buffer pointer math,reinterpret_cast,#pragma once, JSI multiple-inheritance, opinionated/noisy checks) is documented inline with rationale. Analyzes only this package's owncpp/headers; third-party is-isystem.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.The fixes each group surfaced are split across the per-group commits (scoped
DTypeenum, member-init lists,explicitctor,const-correctness cleanup,use-auto/emplace/cmp_*modernizations,NOLINTs for the intentional cases).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 injected finding.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