From 75b66e2b2adce9a28a000422ef3bf9988b628dd2 Mon Sep 17 00:00:00 2001 From: helly25 <6420169+helly25@users.noreply.github.com> Date: Sat, 8 Aug 2026 18:10:03 +0100 Subject: [PATCH] Raise the cognitive-complexity threshold to 30 and clear the rest 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==`; 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> --- .clang-tidy | 2 +- mbo/diff/impl/diff_myers.cc | 4 +++ mbo/diff/internal/output.cc | 9 +++++++ mbo/mope/mope.cc | 3 +++ mbo/mope/mope_main.cc | 52 +++++++++++++++++++++++-------------- tools/clang_tidy.sh | 30 ++++++++++++++++++++- 6 files changed, 78 insertions(+), 22 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 0646e861..06823502 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -110,7 +110,7 @@ CheckOptions: - key: readability-implicit-bool-conversion.AllowPointerConditions value: '1' - key: readability-function-cognitive-complexity.Threshold - value: '25' + value: '30' - key: readability-braces-around-statements.ShortStatementLines value: '1' - key: readability-identifier-length.IgnoredLoopCounterNames diff --git a/mbo/diff/impl/diff_myers.cc b/mbo/diff/impl/diff_myers.cc index 4b3486af..188312c9 100644 --- a/mbo/diff/impl/diff_myers.cc +++ b/mbo/diff/impl/diff_myers.cc @@ -181,6 +181,10 @@ void DiffMyers::Loop() { } } +// The forward/reverse D-path search is one algorithm from Myers' paper, and the bookkeeping (the two +// diagonal windows, the parity rule and the overlap test) only makes sense read together. Splitting +// it into helpers would hide the correspondence to the paper without making any part simpler. +// NOLINTNEXTLINE(readability-function-cognitive-complexity) DiffMyers::Snake DiffMyers::FindMiddleSnake(const Span& span) { // The span was trimmed: both sides are non-empty and neither the first nor // the last lines match, so the minimal cost is >= 2 and the first overlap diff --git a/mbo/diff/internal/output.cc b/mbo/diff/internal/output.cc index 904e38e5..d987b1cb 100644 --- a/mbo/diff/internal/output.cc +++ b/mbo/diff/internal/output.cc @@ -74,6 +74,10 @@ void AppendUnified( } } +// As with AppendSideBySide: the branching enumerates the context format's cases (change blocks +// showing '!' on both sides versus lone '-' / '+' runs), which is the specification this function +// implements rather than incidental complexity. +// NOLINTNEXTLINE(readability-function-cognitive-complexity) void AppendContext( std::string& output, const DiffOptions& options, @@ -185,6 +189,11 @@ void AppendNormal( } } +// The branching is the side-by-side format itself: the four row kinds and their gutter markers, +// plus padding and truncation per cell. The emitted text is the contract these functions exist to +// produce, so splitting on a complexity metric would trade a readable format description for +// helpers that can only be understood together. +// NOLINTNEXTLINE(readability-function-cognitive-complexity) void AppendSideBySide( std::string& output, const DiffOptions& options, diff --git a/mbo/mope/mope.cc b/mbo/mope/mope.cc index 7c2b77f2..b18c4d2f 100644 --- a/mbo/mope/mope.cc +++ b/mbo/mope/mope.cc @@ -437,6 +437,9 @@ std::pair Template::MaybeExpandWhiteSpace( : std::make_pair(tag_pos, tag.start.length()); } +// A single scan over the output that dispatches on each tag kind it meets; at 32 it is barely over +// the threshold, and the tag handling reads as one sequence rather than as separable steps. +// NOLINTNEXTLINE(readability-function-cognitive-complexity) absl::Status Template::ExpandInternal(Context& ctx, std::string& output) const { std::string_view pos = output; while (true) { diff --git a/mbo/mope/mope_main.cc b/mbo/mope/mope_main.cc index bcce422c..082ddfd6 100644 --- a/mbo/mope/mope_main.cc +++ b/mbo/mope/mope_main.cc @@ -61,32 +61,44 @@ struct Options { }; namespace { + +// Applies a single `--set==` argument. The key may be a ':' separated section path, in +// which case the value is set on the addressed (possibly nested) section; without a path it becomes +// a global context value. No path component may be empty. +absl::Status ApplySetFlag( + std::string_view set_kv, + mbo::mope::Template& mope_template, + absl::flat_hash_map& context_data) { + static constexpr std::string_view kEmptyPartError = + "No part of the key in `--set==` may be empty if split by ':'."; + const auto [names, value] = std::pair(absl::StrSplit(set_kv, '=')); + std::vector section_names = absl::StrSplit(names, ':'); + const std::string_view key = section_names.back(); + if (key.empty()) { + return absl::InvalidArgumentError(kEmptyPartError); + } + section_names.pop_back(); + if (section_names.size() == 1 && section_names[0].empty()) { + context_data[key].assign(value); // Global context_data + return absl::OkStatus(); + } + auto* section = &mope_template; + for (const std::string_view section_name : section_names) { + if (section_name.empty()) { + return absl::InvalidArgumentError(kEmptyPartError); + } + MBO_ASSIGN_OR_RETURN(section, section->AddSection(section_name)); + } + return section->SetValue(key, value); +} + absl::Status Process(const Options& opts) { auto input = mbo::file::Artefact::Read(opts.template_name); mbo::mope::Template mope_template; // Add `--set` flag values. absl::flat_hash_map context_data; for (const auto& set_kv : absl::GetFlag(FLAGS_set)) { - const auto [names, value] = std::pair(absl::StrSplit(set_kv, '=')); - std::vector section_names = absl::StrSplit(names, ':'); - std::string_view key = section_names.back(); - if (key.empty()) { - return absl::InvalidArgumentError("No part of the key in `--set==` may be empty if split by ':'."); - } - section_names.pop_back(); - if (section_names.size() == 1 && section_names[0].empty()) { - context_data[key].assign(value); // Global context_data - } else { - auto* section = &mope_template; - for (std::string_view section_name : section_names) { - if (section_name.empty()) { - return absl::InvalidArgumentError( - "No part of the key in `--set==` may be empty if split by ':'."); - } - MBO_ASSIGN_OR_RETURN(section, section->AddSection(section_name)); - } - MBO_RETURN_IF_ERROR(section->SetValue(key, value)); - } + MBO_RETURN_IF_ERROR(ApplySetFlag(set_kv, mope_template, context_data)); } // Read `--ini` file if present. const std::string ini_filename = absl::GetFlag(FLAGS_ini); diff --git a/tools/clang_tidy.sh b/tools/clang_tidy.sh index 379a8dda..74b85b77 100755 --- a/tools/clang_tidy.sh +++ b/tools/clang_tidy.sh @@ -109,7 +109,35 @@ done # Nothing to check (pre-commit may invoke with no matching files). [ "${#}" -gt 0 ] || exit 0 +# Checks that carry no meaning in test code, disabled for `*_test.cc` only. Doing +# it here rather than with a NOLINT per test keeps one statement of the rule +# instead of a comment repeated on every test, and new tests are covered without +# anyone remembering to annotate them. +# * readability-function-cognitive-complexity: a gtest TestBody's score comes +# from ASSERT_*/EXPECT_* macros expanding to branches, not from logic that +# could be refactored. Test bodies reached 122 against a threshold of 30. +# `--checks` APPENDS to the `Checks` in .clang-tidy (it does not replace it), so +# every other check still applies to tests. +readonly TEST_DISABLED_CHECKS='-readability-function-cognitive-complexity' + +declare -a SOURCES=() +declare -a TESTS=() +for FILE in "${@}"; do + case "${FILE}" in + *_test.cc | *_test.cpp | *_test.cxx) TESTS+=("${FILE}") ;; + *) SOURCES+=("${FILE}") ;; + esac +done + # Report only: --header-filter restricts diagnostics to this repo's own headers # (not the toolchain's force-included / system headers), -p points at the compile # DB. WarningsAsErrors in .clang-tidy makes any finding a non-zero exit. -exec "${CLANG_TIDY}" --header-filter='(^|/)mbo/' -p . "${@}" +# Both groups must run, and a finding in either has to fail, so no `exec` here. +STATUS=0 +if [ "${#SOURCES[@]}" -gt 0 ]; then + "${CLANG_TIDY}" --header-filter='(^|/)mbo/' -p . "${SOURCES[@]}" || STATUS=1 +fi +if [ "${#TESTS[@]}" -gt 0 ]; then + "${CLANG_TIDY}" --header-filter='(^|/)mbo/' --checks="${TEST_DISABLED_CHECKS}" -p . "${TESTS[@]}" || STATUS=1 +fi +exit "${STATUS}"