Move clang-tidy from trunk to a pre-commit hook, and fix the compile DB - #270
Merged
Conversation
trunk pinned clang-tidy 16, which mis-parses this C++23 codebase, and its `--export-fixes` runs rewrote the working tree with build-breaking "fixes". trunk.io 403s any modern clang-tidy download, so a version bump there is not possible. - Drop clang-tidy from trunk (.trunk/trunk.yaml). - Add tools/clang_tidy.sh, resolving the hermetic clang-tidy (>= 18, mirroring clang_format.sh's ladder) and reporting only, never --fix. The version gate matters: a plain `clang-tidy` on PATH may well be trunk's 16. - Add an opt-in `clang-tidy` pre-commit hook, plus a CI job that builds the compile DB, uploads it as an artifact and runs just that hook. The main pre-commit job skips it. .clang-tidy carried two latent bugs: `WarningsAsErrors: true` is a check glob, not a bool, so it matched nothing and findings never escalated; and `bugprone-signed-char-misuse.CharTypdefsToIgnore` was misspelled, leaving the option dead. Both fixed. `llvm-header-guard` and `llvm-prefer-static-over-anonymous-namespace` are disabled as they contradict STYLE_CPP.md. compile_commands-update.sh was generating the wrong compile DB entirely: `--config=clang` only configures the build of the extractor tool and never reaches the aquery it runs internally, so every recorded command named the autodetected Apple clang while clangd and clang-tidy used the hermetic one. It now passes --bcce-compiler explicitly, plus an -isysroot copt on Darwin (the hermetic clang ships libc++ but no system C headers). It also adopts --bcce-prefer-target-config, dropping 196 duplicate exec-configuration entries, which requires the extractor pin bump to 6eb3ff1. The CI job is continue-on-error for now: WarningsAsErrors: '*' against a tree that is not yet clean would block everything. Promoting it to a real gate means re-tuning .clang-tidy for the noisy new-in-22 checks, sweeping the residual findings, then dropping both `stages: [manual]` and continue-on-error. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
Job-level continue-on-error masked everything, including a failure to build the compile DB -- the one thing in that job that must gate. Move it to the pre-commit step, so only the not-yet-clean findings are tolerated while a broken extractor or toolchain fails the job. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
Two defects the first CI run of the clang-tidy job exposed.
The compile DB was extracted without the generated headers existing, so
the extractor could not resolve translation units including them
("hash_mangle_seed_gen.h is missing") and they landed in the DB degraded
-- which later reads as clang-tidy findings rather than the build artifact
gap it really is. Build them first. Generated headers are the only build
output a TU needs to parse, so this builds just those (3 targets, ~7s)
rather than all of //..., with a query that stays correct as generated
files are added and a guard that fails loudly if it ever matches nothing.
clang-tidy 22.1.8 also SEGFAULTS (exit -11) in
abseil-unchecked-statusor-access: its dataflow analysis crashes on
mbo/strings/strip.cc, reproducibly on both Linux and macOS. Disable the
check; with it off the same file exits normally.
Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
The job finished, but its lint step took 24 minutes: ~50s per translation unit over all 81 sources. More parallelism is not the lever -- pre-commit already splits the files across every core (the run showed 21 files per invocation, i.e. 4 concurrent chunks on a 4-core runner), so the runner is already saturated and adding xargs -P inside the hook would only oversubscribe it. Scope the work instead: a branch lints only the sources it changed, while `main` still lints the whole tree after merge. Requires fetch-depth 0 to diff against main. Known gap, accepted deliberately: editing only a header changes findings in the sources that include it, and none of those appear in the branch's changed-file set. The post-merge run on `main` catches that. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25
enabled auto-merge (squash)
August 8, 2026 14:06
Fab-Cat
approved these changes
Aug 8, 2026
A branch run lints just the sources that branch changed, so its compile DB describes a near-empty lint and is not worth storing. The `main` run is the one that lints the whole tree, and its DB is what reproduces a finding locally against the exact same commands. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
Artifacts are otherwise only visible in the panel at the bottom of a run page, unlabelled. Write a job summary instead: DB size, how many duplicate entries survived deduplication, the lint scope actually used, and -- on main, where the artifact exists -- a direct link to it. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25
added a commit
that referenced
this pull request
Aug 8, 2026
xff's copy had grown several clarifications worth having here, adapted to mbo's names and to what mbo actually has. Notably one is a REVERSAL of the existing rule. The doc said "a by-value std::string_view is never const"; it now follows ordinary const-correctness - const when the view is never mutated, non-const exactly when it is resliced or reassigned - because misc-const-correctness (enabled) flags a never-mutated view that is not const. The corollary and a worked example are included, plus the companion rule that a read-only string parameter is std::string_view by value rather than const std::string&. Also adopted: trailing commas on every element of a registry-style table; "a value or error type IS absl::StatusOr<T>"; matchers named unqualified via `using`; mbo::testing::EqualsText with a DropIndent raw-string golden for multi-line text; ElementsAreArray with a trailing comma for long matcher lists; SizeIs/IsEmpty on the container instead of matching an extracted .size()/.empty(); and bashtest's content matchers with the whole-text anchoring caveat. The clang-tidy paragraph was also stale - it still claimed CI does not run clang-tidy, which #270 changed. It now describes the dedicated CI job, the changed-files-on-a-branch vs whole-tree-on-main scoping, and that it is report-only until the sweep lands. Deliberately not copied: xff's XFF_ macro prefix, xff/ paths and flags, its C++23 baseline, its com_helly25_bashtest repo name (mbo uses the default), its no-shell-grep-in-bashtests hook (mbo has none), and its diff_golden.bzl - the golden-file guidance instead points at mbo's own //mbo/diff:diff.bzl. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25
added a commit
that referenced
this pull request
Aug 9, 2026
xff's copy had grown several clarifications worth having here, adapted to mbo's names and to what mbo actually has. Notably one is a REVERSAL of the existing rule. The doc said "a by-value std::string_view is never const"; it now follows ordinary const-correctness - const when the view is never mutated, non-const exactly when it is resliced or reassigned - because misc-const-correctness (enabled) flags a never-mutated view that is not const. The corollary and a worked example are included, plus the companion rule that a read-only string parameter is std::string_view by value rather than const std::string&. Also adopted: trailing commas on every element of a registry-style table; "a value or error type IS absl::StatusOr<T>"; matchers named unqualified via `using`; mbo::testing::EqualsText with a DropIndent raw-string golden for multi-line text; ElementsAreArray with a trailing comma for long matcher lists; SizeIs/IsEmpty on the container instead of matching an extracted .size()/.empty(); and bashtest's content matchers with the whole-text anchoring caveat. The clang-tidy paragraph was also stale - it still claimed CI does not run clang-tidy, which #270 changed. It now describes the dedicated CI job, the changed-files-on-a-branch vs whole-tree-on-main scoping, and that it is report-only until the sweep lands. Deliberately not copied: xff's XFF_ macro prefix, xff/ paths and flags, its C++23 baseline, its com_helly25_bashtest repo name (mbo uses the default), its no-shell-grep-in-bashtests hook (mbo has none), and its diff_golden.bzl - the golden-file guidance instead points at mbo's own //mbo/diff:diff.bzl. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
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.
Adopts helly25/xff#386 here, plus two compile-DB fixes found while verifying it.
clang-tidy out of trunk
trunk pinned clang-tidy 16, which mis-parses this C++23 codebase, and its
--export-fixesruns rewrote the working tree with build-breaking "fixes". trunk.io 403s any modern clang-tidy download, so a version bump there is not possible. Confirmed here:which clang-tidyresolves to.trunk/tools/clang-tidy= LLVM 16.0.3, while the hermetic toolchain is 22.1.8..trunk/trunk.yaml.tools/clang_tidy.sh: resolves the hermetic clang-tidy (mirroringclang_format.sh's ladder), gates on >= 18, and reports only — never--fix. The version gate is load-bearing: a plainclang-tidyonPATHmay well be trunk's 16, and it is rejected (verified).clang-tidypre-commit hook, plus a CI job that builds the compile DB, uploads it as an artifact, and runs just that hook. The mainpre-commitjob skips it..clang-tidylatent bugsWarningsAsErrors: true— a check-name glob, not a bool, so it matched nothing and findings never escalated. Now'*'.bugprone-signed-char-misuse.CharTypdefsToIgnore— misspelled (missinge), so the option was dead. Surfaced by--verify-config.llvm-header-guardandllvm-prefer-static-over-anonymous-namespacedisabled: they contradictSTYLE_CPP.md. The former, with no header root, suggestsUSERS_MARCUS_DOCUMENTS_..._SCOPED_STREAM_Hfor every header.compile_commands-update.shwas generating the wrong compile DBbazel run @…//:refresh_all --config=clangdoes nothing useful: that flag only configures the build of the extractor tool, and never reaches theaqueryit runs internally. Every recorded command therefore named the autodetected Apple clang, while clangd and clang-tidy used the hermetic one — which is why clang-tidy died on'concepts' file not found.--bcce-compiler=<hermetic clang++>explicitly (after--, orbazel runeats it).--bcce-copt=-isysroot$(xcrun --show-sdk-path): the hermetic clang ships libc++ but no system C headers, so<locale>failed ontime.h.--bcce-prefer-target-config(Add --bcce-prefer-target-config to drop redundant exec-config commands bazel-compile-commands-extractor#29), dropping 196 duplicate exec-configuration entries — 2223 -> 2027 entries over an unchanged set of 2027 files. Needs the extractor pin bump to6eb3ff1.This fixes clangd too, which had the same mismatch.
Test
pre-commit run -agreen.--override_module):dropped 196 duplicate source entries. Exec-only files (mbo/mope/ini.h,mbo/diff/internal/update_absl_log_flags.h) are retained;status_builder.ccwent 2 entries -> 1.PATH), and the hook does not run at the default commit stage.Deliberately not a gate yet
The CI job is
continue-on-error: trueand the hook isstages: [manual].WarningsAsErrors: '*'against a tree that is not yet clean would block nearly every commit — one sampled file alone yields ~500 findings, ~80% fromcppcoreguidelines-pro-bounds-avoid-unchecked-container-access, which flags everyoperator[].Promoting it to a real gate: re-tune
.clang-tidyfor the noisy new-in-22 checks, sweep the residual findings, then drop bothstages: [manual]andcontinue-on-error. Recorded in the hook description.Note the whole recipe is verified on macOS; the ubuntu runner path gets its first exercise from this PR's CI, which reports rather than blocks.