Raise the cognitive-complexity threshold to 30 and clear the rest - #273
Merged
Conversation
helly25
force-pushed
the
clang_tidy_complexity
branch
from
August 8, 2026 17:19
6d87797 to
341f6fa
Compare
The threshold was never the real problem: of 37 findings at 25, only 9
were production functions -- the other 28 were gtest `TestBody`, where
the complexity is ASSERT_* macros expanding to branches rather than logic
anyone should refactor. No threshold fixes that (TestBody reached 122),
and IgnoreMacros was measured too blunt: it silenced all 37, including
the genuine ones.
So: threshold 30, which clears the four borderline production functions
(26, 29, 29, 30) while still flagging 32/34/35/39/54.
Tests are handled once, in tools/clang_tidy.sh, which now lints
`*_test.cc` with the check appended as disabled. clang-tidy has no way to
express this in .clang-tidy -- its schema has no per-file-pattern section
and its only granularity is per directory, which cannot separate tests
that live beside the code they test. Doing it in the hook keeps one
statement of the rule instead of the same NOLINT comment on every test,
and covers new tests without anyone remembering to annotate them.
`--checks` appends to the configured `Checks`, so tests keep every other
check.
The five production functions, case by case:
* mope_main.cc Process (39) - split. Its complexity was almost entirely
one nested block parsing `--set=<key>=<value>`; extracted as
ApplySetFlag. Covered by //mbo/mope/tests/args, including the nested
`section:enabled,:config_start=25` path.
* FindMiddleSnake (54), AppendSideBySide (35), AppendContext (34),
ExpandInternal (32) - suppressed, each with its own reason. The first
is one algorithm from Myers' paper whose bookkeeping only reads as a
whole; the diff formatters' branching IS the output format they
exist to specify.
Verified that the relaxation is scoped: with its NOLINT removed a
production file reports the finding again, while test files and mixed
invocations report none.
Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25
enabled auto-merge (squash)
August 8, 2026 18:17
Fab-Cat
approved these changes
Aug 8, 2026
helly25
force-pushed
the
clang_tidy_complexity
branch
from
August 8, 2026 18:19
8b96584 to
75b66e2
Compare
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
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.
Third clang-tidy triage PR. Clears all 37
readability-function-cognitive-complexityfindings, touching 6 files instead of 20.The threshold was not the real problem
Of the 37 findings at threshold 25, only 9 were production functions. The other 28 were gtest
TestBody, where the complexity isASSERT_*macros expanding to branches — not logic anyone should refactor.No threshold fixes the tests —
TestBodyreaches 122 — and one high enough to clear them makes the check decorative for production code.IgnoreMacroslooked like the obvious lever but measured too blunt: it silences all 37 including the genuine ones, because it excuses any function touching a macro rather than discounting macro-contributed complexity.Threshold 30, and tests handled once
Threshold 30 clears the four borderline production functions (26, 29, 29, 30) that are dense-but-fine, while still flagging 32/34/35/39/54.
Tests are relaxed in
tools/clang_tidy.sh, which lints*_test.ccwith the check appended as disabled.This cannot be expressed in
.clang-tidy: its schema is exactlyChecks,CheckOptions,WarningsAsErrors,HeaderFilterRegex,ExcludeHeaderFilterRegex,HeaderFileExtensions,ImplementationFileExtensions,FormatStyle,SystemHeaders,User— there is no per-file-pattern section, and its only granularity is per directory, which cannot separate tests that live beside the code they test.Doing it in the hook means one statement of the rule instead of the same NOLINT comment on 24 tests, and new tests are covered without anyone remembering to annotate them.
--checksappends to the configuredChecks(verified), so tests keep every other check.The five production functions, case by case
mope_main.cc Process(39) — split. Its complexity was almost entirely one nested block parsing--set=<key>=<value>(a section-path loop inside a branch inside a loop). Extracted asApplySetFlag, leavingProcessa linear pipeline.FindMiddleSnake(54) — suppressed. The forward/reverse D-path search is one algorithm from Myers' paper; the two diagonal windows, parity rule and overlap test only make sense read together.AppendSideBySide(35),AppendContext(34) — suppressed. The branching is the diff output format — row kinds, gutter markers, padding, truncation. That text is the contract these functions exist to produce.ExpandInternal(32) — suppressed. Two over the threshold; one scan dispatching on tag kind.Each suppression carries its own reason rather than a boilerplate one.
Test
NOLINTremoved a production file reports the finding again (1), restored it reports none, and test files and mixed test+production invocations report none. A production file still gets fully linted (149 findings from other checks).bazel test --config=clang //mbo/mope/...:all //mbo/diff:all— 12/12 pass. The--setrefactor is covered by//mbo/mope/tests/args, including the nestedsection:enabled,:config_start=25path that exercises the extracted branch.pre-commit run -agreen.