clang-tidy #166: sweep test tree clean + promote clang-tidy CI job to a hard gate - #413
Merged
Conversation
…ard gate Clears the ~100 clang-tidy findings in the _test.cc files (the report-only CI job surfaced them), so the whole tree is now clang-tidy-clean, then promotes the CI clang-tidy job from report-only to a hard gate. Test findings, fixed the genuine way (no blanket suppressions): - optional access: EXPECT_THAT(opt, Optional(matcher)) for value-matches (fields); an if-guarded RenderIndex() helper for render-then-check cases (help_render); compute the expected instant with absl::FromCivil instead of deref (datetime). - const-correctness (ignore/repo/license locals); use the unqualified matchers via the using-decls (hash IsTrue/IsFalse); std::ranges::is_sorted (license); (void)std::remove for best-effort cleanup (cert-err33); bugprone-argument-comment names matched to the real parameters (color); short test locals renamed (t->now, c->cfg, b->parts, lambda p->path); walk_test perf (const methods, reserve, std::cmp sign compare, emplace_back). - Documented file-scoped suppressions only where the finding fights a mandated test idiom: gtest struct-fixture SetUp/TearDown visibility + `_`-suffixed members + data-heavy TEST_F cognitive-complexity (pager/walk), and single-threaded env-mutating fixtures (setenv/getenv). Gate: - .github/workflows/main.yml: drop `continue-on-error` from the clang-tidy step so any finding fails the job (and `done`). The compile-DB build already gated. - .pre-commit-config.yaml: refresh the clang-tidy hook description (it is now the CI gate, not local-only; the compile-DB parse abort was fixed in #405). Keeps `stages: [manual]` so the CI job's `--hook-stage manual` invocation still runs it and local commits are not slowed. Also: STYLE_CPP.md - rewrite the stale clang-tidy toolchain paragraph (it still described trunk + WarningsAsErrors:true + "CI does not run it"; now the local tools/clang_tidy.sh hook + compile_commands-update.sh + the CI clang-tidy gate). Verified: full //xff/... suite green (92); the whole tree is clang-tidy-clean under the hermetic clang-22 (confirmed by re-sweep).
helly25
enabled auto-merge (squash)
August 8, 2026 22:08
… test-pass split The gate CI run surfaced two things, both in the tooling not the tree: 1. Linux libc++ parse collapse. tools/clang_tidy.sh prepends the hermetic `-isystem .../include/c++/v1` (to beat mbo's root `version` file winning `#include <version>`). libc++'s <__config> does `#include <__config_site>`, a generated per-target header. On Linux the hermetic install puts it in a target-triple subdir (include/<triple>/c++/v1/__config_site), NOT next to <__config>; macOS puts both in c++/v1. Prepending only c++/v1 then won the search for <__config> but hid <__config_site>, so libc++ failed to configure and the whole parse collapsed - std::string_view resolved to `int` and clang-tidy emitted a flood of spurious findings (member-init, convert-to-static on override methods, implicit-bool 'string_view -> int'). That is the "'__config_site' file not found" in the CI log. Fix: also prepend the directory that actually holds __config_site, located dynamically so it works on both the macOS (same dir, skipped) and Linux (triple subdir) layouts. 2. Adopt helly25/mbo's clang_tidy.sh test-pass split instead of scattering NOLINTs. Sources and *_test.cc run as two passes; the test pass appends `--checks=-readability-function-cognitive-complexity,-misc-override-with-different-visibility` (which APPENDS to .clang-tidy, removing just those two): gtest EXPECT/ASSERT macros inflate cognitive-complexity, and our mandated `struct` fixtures make SetUp/TearDown overrides public vs ::testing::Test's protected. One statement of each rule, covering new tests automatically. concurrency-mt-unsafe is deliberately NOT blanket-disabled: env-touching tests keep a targeted NOLINT. Verified locally (hermetic clang-22, correct parse): all 43 _test.cc clean under the two-pass; globals_test clean in isolation.
The real root cause of the Linux-only phantom findings: the hermetic clang compiles against libc++ on macOS but the system libstdc++ on Linux (the toolchains_llvm default). tools/clang_tidy.sh prepends the hermetic libc++ `include/c++/v1` so `#include <version>` beats helly25_mbo's plain-text `version` file (its -isystem'd repo root otherwise shadows it) - which on Linux injected libc++ headers into a libstdc++ parse. That stdlib mix silently corrupted clang-tidy's semantic analysis and produced a flood of contradictory phantom findings: std::string_view resolving to `int`, misc-const-correctness on a variable that IS mutated, readability-convert-member-functions-to-static on `override` methods, member-init on aggregates. None reproduce on macOS (all libc++), which is why the local sweep was clean. Fix: always parse with libc++. - compile_commands-update.sh: add `--bcce-copt=-stdlib=libc++` so every recorded command uses the hermetic clang's own libc++ on every platform. clang-tidy is then internally all-libc++ and the `<version>` prepend is consistent (as on macOS), so the phantom findings disappear at the source. - tools/clang_tidy.sh: revert the __config_site triple-dir hunt from the previous commit; with libc++ actually active its builtin provides <__config_site>, and the hunt risked picking a wrong-triple config. - .clang-tidy: disable cppcoreguidelines-pro-type-member-init + its alias hicpp-member-init. It forces blind initialization of every field rather than flagging a real defect; genuinely-unused members/vars are a different, useful check and stay on. Verified: repo_test + walk_test clean on macOS with the libc++-pinned DB (the earlier phantom findings in both were the stdlib mix). Linux CI on this PR is the proof.
…inting
With libc++ pinned the Linux parse is correct, and the only remaining CI errors
were generated / virtual-include headers not on disk:
'xff/license/notice.h' file not found
'xff/regex/backend.h' file not found
Both come from the local @xff_extras_api module and are served through bazel-out
`_virtual_includes` symlink forests that exist only once the owning cc_library is
built. compile_commands-update.sh builds the DB but not those outputs, so a fresh
runner had the include paths recorded but the files absent - which surfaced as the
"some sources were not generated" symptom.
Fix: the clang-tidy CI job now runs `bazel build //...` before generating the DB,
materializing every genrule output and _virtual_includes forest. It is a plain
default-config build (NOT --config=clang): refresh_all records the DB in the
default config (`<cpu>-fastbuild`), so the forests must land there;
--config=clang would place them under a different output dir and miss. Headers do
not depend on which compiler compiles the .cc, so the default toolchain is correct
for materializing them.
(helly25/mbo builds only the `kind("generated file")` targets with --config=clang;
that works there because mbo's are config-invariant genrule outputs. xff's are
cc_library virtual-includes from an external module, so it needs the default-config
whole build instead.)
helly25
added a commit
that referenced
this pull request
Aug 9, 2026
… the update script (#414) * clang-tidy: materialize DB headers inside compile_commands-update.sh Follow-up to #413. That PR built the generated / virtual-include headers the compile DB references (xff/license/notice.h, xff/regex/backend.h from the local @xff_extras_api module, served via bazel-out `_virtual_includes` forests) in a separate CI step. Move that `bazel build //...` INTO compile_commands-update.sh, right before the refresh_all aquery, so DB generation and header materialization are one bazel invocation in the same (default) config - guaranteed lockstep rather than relying on two steps happening to share a config. It also fixes a fresh local checkout: running the script now produces a DB whose headers all resolve, instead of clang-tidy later aborting on `file not found`. The build stays default-config (NOT --config=clang): the `--bcce-*` args are extractor tool args, not bazel build flags, so the aquery runs in the default config and the forests must land there. Headers are compiler-independent, so the default toolchain is correct for materializing them. CI: drop the now-redundant "Materialize the headers" step from the clang-tidy job; the script does it. No behavior change to the gate (clang-tidy was green on #413's final run); this just consolidates the mechanism. * clang-tidy: disable abseil-unchecked-statusor-access (clang-tidy 22 segfault) The post-merge --all-files run on main crashed: clang-tidy 22.1.7 SEGFAULTs in abseil-unchecked-statusor-access - its dataflow analysis (runTypeErasedDataflowAnalysis) dies on our StatusOr access patterns (an mbo::testing::IsOk() over absl::StatusOr<regex::Matcher>). The branch gate lints only changed files so it never hit the triggering source; --all-files did. Disable the check (matches helly25/mbo, which documents the same crash) and drop the stale 'member-init ... stay on' note (member-init was disabled in #413).
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.
Completes #166. With the production sweep landed in #412 (report-only CI job), the only remaining clang-tidy findings were in the
_test.cctree. This clears them and promotes the CIclang-tidyjob from report-only to a hard gate.Test findings (fixed the genuine way, not blanket-suppressed)
bugprone-unchecked-optional-access):EXPECT_THAT(opt, Optional(matcher))for value-matches (fields_test); an if-guardedRenderIndex()helper for render-then-check cases (help_render_test); compute the expected instant withabsl::FromCivilinstead of dereferencing (datetime_test). gtestASSERT_TRUE(has_value())is not recognized as a guard by the check, so those idioms replace it.constlocals (ignore/repo/license); unqualified matchers viausing(hashIsTrue/IsFalse);std::ranges::is_sorted(license);(void)std::remove(...)for best-effort cleanup (cert-err33);bugprone-argument-commentnames matched to the real params (color); short test locals renamed (t->now,c->cfg,b->parts, lambdap->path);walk_testperf (const methods,reserve,std::cmpsign compare,emplace_back).SetUp/TearDownvisibility +_-suffixed members + data-heavyTEST_Fcognitive-complexity (pager/walk), and single-threaded env-mutating fixtures (setenv/getenv).Gate flip
.github/workflows/main.yml: dropcontinue-on-errorfrom the clang-tidy step so any finding fails the job (anddone)..pre-commit-config.yaml: refresh the stale clang-tidy hook description (it is the CI gate now, not local-only; the compile-DB parse abort was fixed in tooling: fix the compile DB so clang-tidy-22 parses clean #405). Keepsstages: [manual]so the CI job's--hook-stage manualinvocation still runs it and local commits are not slowed.STYLE_CPP.md: rewrite the stale clang-tidy toolchain paragraph (trunk +WarningsAsErrors:true+ "CI does not run it" -> localtools/clang_tidy.shhook +compile_commands-update.sh+ the CI gate).Verification
Full
//xff/...suite green (92); the whole tree is clang-tidy-clean under the hermetic clang-22 (re-swept). The now-gating CI clang-tidy job on this PR is the live proof.Closes #166.