Thanks for pushing this forward — I like the direction. I do think we can make the proposal both more precise and more effective by tightening the “what blocks” semantics, and by making the results show up where review actually happens.
The first thing I’d change is the way we decide whether the linter “failed”. Right now the helper seems to infer failure from “did the tool print anything to stdout/stderr”, which is the wrong contract for clang-tidy (and for the whole idea of mixing blocking vs informational diagnostics). See code-lint-helper.py around here: llvm-project/llvm/utils/git/code-lint-helper.py at main · llvm/llvm-project · GitHub. Output can be perfectly valid in a non-blocking mode (warnings, notes, noisy checks we’re still evaluating), so using “any output == fail” makes the rollout policy ambiguous and brittle.
A clean way to fix this is to make the blocking policy come from clang-tidy’s native configuration, and have CI only look at the exit code. Clang-tidy already supports granular “these warnings are errors” via WarningsAsErrors in .clang-tidy. That’s also the intuitive single source of truth for both CI and local runs: the wrapper/script doesn’t need to encode policy, it just runs clang-tidy and blocks only if the configured “errors” are present (non-zero exit).
On the “non-blocking vs blocking” question: I don’t think it should be one mode forever. Non-blocking is great as an adoption ramp, but long-term it’s valuable to explicitly split checks into two buckets: (1) checks we want to enforce because they’re low false-positive / largely mechanical, and (2) checks that are useful but more heuristic/noisy, which can remain informational until proven stable. This is also where the stylistic-check objection can be addressed constructively: rather than “no style checks”, we can say “style checks are fine, but they graduate to blocking only when they are useful and demonstrate low false-positive rates”. For LLVM in particular, the deterministic style/convention checks are exactly the ones that save reviewer bandwidth (and avoid humans repeating the same comments), but we should earn the right to block merges with them.
To make “graduation” concrete, we could use a simple measurable rule: start a check as non-blocking, and promote it to blocking once its observed false-positive rate drops below N% (with N agreed by the community and allowed to differ by check/component). In practice, N should likely be quite low for checks that are expected to be deterministic, and only higher for inherently heuristic checks if a component explicitly opts in. False positives can be handled with targeted suppressions (e.g. // NOLINTNEXTLINE(<check-name>)), but the main failure mode to watch for is “just-in-case” or copy-pasted suppressions that end up masking real issues. To keep that under control, it helps to make suppressions auditable via clang-tidy itself — either through configuration options or a dedicated check: forbid non-targeted NOLINTs, and treat it as an error when a targeted suppression doesn’t actually suppress any diagnostic. That way stale or unnecessary suppressions get cleaned up automatically instead of accumulating over time.
One more thing that would materially improve this: the results shouldn’t live as raw CI text hidden behind spoilers. It’s much more useful if clang-tidy shows up as an actual GitHub PR review with inline annotations and (where possible) apply-fix suggestions — like GitHub - ZedThree/clang-tidy-review: Create a pull request review based on clang-tidy warnings. That makes issues visible to all reviewers, reduces duplicate human comments, and makes it easier to have the right conversation on the borderline/noisy diagnostics (especially while some checks are still informational).
Finally, I’d make the rollout plan less open to interpretation with explicit steps:
-
Fix the CI gating semantics for clang-tidy to use WarningsAsErrors (so clang-tidy returns non-zero only for enforced diagnostics), and have the calling code block based on exit status rather than “any output”.
-
Add suppression auditing as part of the pipeline policy surface (this likely needs new work): implement a clang-tidy check or small companion linter that (a) forbids non-targeted NOLINT/NOLINTNEXTLINE and (b) errors on suppressions that don’t actually suppress any diagnostic.
-
Enable clang-tidy as non-blocking in components that already have a .clang-tidy config, to validate infra/CI load and catch integration issues early.
-
In parallel (component rollout):
- Components with existing configs: promote low-FP checks to blocking where appropriate via WarningsAsErrors.
- Components without configs: start with 1–few “always correct” checks (e.g.
llvm-include-guard) and make those blocking from day one; then expand incrementally.
-
Implement GitHub PR review reporting for clang-tidy diagnostics (parsing clang-tidy YAML output etc.), along the lines of GitHub - ZedThree/clang-tidy-review: Create a pull request review based on clang-tidy warnings.
-
Once coverage is broad, move common config to the repo root to avoid divergence, using InheritParentConfig so components can override/add as needed.
Also, tracking this via GitHub issues would help: one issue per rollout step, and (where useful) issues per component–check (or component–check-family) so decisions are documented and discussion stays targeted. Many of those could be ‘good first issue’ candidates for incremental enablement work.
Stepping back, the main concern is keeping the behavior understandable over time: it should be obvious what is informational vs what is enforced, and that shouldn’t depend on CI log parsing or wrapper heuristics. Two things help a lot here: explicit enforcement rules and review-visible reporting. With those in place, the mix of blocking vs non-blocking checks can be tuned incrementally as we learn where false positives show up.