Repository navigation
Enable clang-tidy's readability-function-cognitive-complexity check #7358
Description
Activity
The check was enabled in #7995 with
Threshold=50andIgnoreMacros=true, and baselineNOLINTs were added for everything above that. These are the functions currently over 50, all suppressed (16 functions, 15NOLINTsites), measured at bc7f0cf by running only this check with theNOLINTs neutralised:Score Function Location 277 init_handlerssamples/apps/logging/logging.cpp:593 266 init_handlerssrc/node/rpc/node_frontend.h:484 127 init_proposals_handlerssrc/node/gov/handlers/proposals.h:473 125 initiate_join_unsafe(+ nested lambdas, 89 and 61)src/node/node_state.h:1382 109 init_service_state_handlerssrc/node/gov/handlers/service_state.h:422 102 do_execute_requestsrc/js/registry.cpp:58 90 process_command_innersrc/node/rpc/frontend.h:703 69 init_ack_handlerssrc/node/gov/handlers/acks.h:28 69 / 66 fill_range_response_from_file/init_file_serving_handlerssrc/node/rpc/file_serving_handlers.h:173 / :552 66 setup_basic_hookssrc/node/node_state.h:3277 55 ProgrammabilityHandlersconstructorsamples/apps/programmability/programmability.cpp:281 84 / 53 test cases (unit tests only) src/node/test/historical_queries.cpp:1293, src/indexing/test/indexing.cpp:460 A function's score includes the bodies of lambdas defined inside it, so the
init_*handlersfunctions are large mostly because their endpoint handlers are written inline. The two test cases are in unit tests, which CI doesn't run clang-tidy on.Remaining work (as of 2026-10-06)
The check is enabled by #7995 (
Threshold=50,IgnoreMacros=true). The remaining work is to remove the last baseline suppressions through behaviour-preserving refactors, without raising the threshold.Since the first inventory, #8482, #8484, #8486, #8487, #8488, #8489, #8504, #8508, and #8509 have removed 10 of the 15 original suppression sites. Most of those removals were mechanical handler or callback extractions. Each of the four functions left needs real control-flow decomposition. They account for five suppression sites across four files: one
NOLINTNEXTLINEper function, plus aNOLINTBEGIN/ENDaround the join response callback.Scores were measured on
mainat 1688a7f with clang-tidy 18.1.8, using the repository.clang-tidywith suppressions disabled. They are unchanged from the original inventory.Score Function Where the complexity is Suggested decomposition Overlapping open PRs 125 (response callback 89, task 61) NodeState::initiate_join_unsafe, src/node/node_state.h:1382117 of 125 comes from the inline response callback and join-response task. The task alone scores 61, mainly from trusted-join completion and snapshot install (29), 4xx handling and the snapshot fetch (13), transport errors (7), and redirects (6). Move the task body into a member function, then split it by outcome: transport error, 4xx/ StartupSeqnoIsOld, redirect, and trusted completion. Moving it without splitting still leaves 61. Preserve the in-flight gate reset, avoidance ofNodeState::lockon the libuv thread, the by-valuetarget_address, retryable-vs-fatal classification, and state-transition order.#8490 (draft) changes one line in the snapshot-install path. 102 BaseDynamicJSEndpointRegistry::do_execute_request, src/js/registry.cpp:58Conversion of the handler return value: body (47) and headers (15). The log/return-exception-details branch appears five times. Extract body, header, and status conversion helpers, plus a single exception-reporting helper. Preserve byte-identical error strings (they currently differ subtly between branches), timeout messages, extension-scope lifetimes, and the ordering of the compaction-conflict rethrow. None 90 RpcFrontend::process_command_inner, src/node/rpc/frontend.h:707Commit/result handling (24), redirect/forwarding (22), nine catch clauses (14), loop guards (13), and authentication/execution (13). Extract the redirect/forwarding decision and the commit-result handling, keeping the retry loop and exception-to-error mapping in place. Preserve retry semantics, Txownership hand-off, acquire ordering of consensus/history loads, and the abort on serialisation failure.#8190 (draft, inactive since August) changes two can_replicate()calls.69 fill_range_response_from_file, src/node/rpc/file_serving_handlers.h:173Rangeheader parsing (44).Extract a pure Rangeparser that returns a range or an error, which also makes the parser unit-testable. Preserve RFC 9110 suffix/open-ended semantics, overflow-safe clamping, empty-range rejection, and the error messages.#8214 (open) renames this function to a templated fill_range_responseand rewrites the read path, so this refactor is best done after #8214 lands.Each PR should remove its suppression and confirm that the original function and every extracted helper score at most 50, rather than moving the suppression elsewhere.
Remaining work (as of 2026-10-07)
#8511 has merged. It removed the suppression from
BaseDynamicJSEndpointRegistry::do_execute_request, whose score dropped from 102 to 19; no extracted helper scores above 19. Three functions remain. They have four suppression sites across three files: oneNOLINTNEXTLINEper function, plus aNOLINTBEGIN/ENDaround the join response callback.The scores below are unchanged from the previous measurement, taken with clang-tidy 18.1.8 using the repository
.clang-tidywith suppressions disabled. Onmainat 5f1a053,initiate_join_unsafeandfill_range_response_from_fileare token-identical to the measured revision.process_command_innerdiffers only by a renamed method call, which does not affect the score.Score Function Where the complexity is Overlapping open PRs 125 (response callback 89, task 61) NodeState::initiate_join_unsafe, src/node/node_state.h:1388117 of 125 comes from the inline response callback and join-response task. The task alone scores 61, mainly from trusted-join completion and snapshot install (29), 4xx handling and the snapshot fetch (13), transport errors (7), and redirects (6). #8490 (draft) changes one line in the snapshot-install path. 90 RpcFrontend::process_command_inner, src/node/rpc/frontend.h:710Commit/result handling (24), redirect/forwarding (22), nine catch clauses (14), loop guards (13), and authentication/execution (13). #8190 (draft, inactive since August) changes two can_replicate()calls.69 fill_range_response_from_file, src/node/rpc/file_serving_handlers.h:173Rangeheader parsing (44).#8214 (open, currently conflicting with main) renames this function to a templatedfill_range_responseand rewrites its read path.Lessons from #8511 for the remaining PRs:
- Move code verbatim: move blocks unchanged into helpers, so they are token-identical to
mainapart from the helper calls and return statements. - Preserve order and lifetimes: keep the order of side effects and the lifetimes of values exactly as they were.
- Verify behaviour: compare tokens against
main, and run a before/after behavioural comparison where practical.
Each PR should remove its suppression and confirm that the original function and every extracted helper score at most 50, rather than moving the suppression elsewhere.
- Move code verbatim: move blocks unchanged into helpers, so they are token-identical to
Diff to enable this looks something like this:
I think we definitely want to ignore macros, because our logging macros are measured as extremely complex but in practice don't make the functions harder to read.
The default threshold is 25, which flags a huge number of functions, and I think is a little low. I suggest we start with a higher threshold, such as 50, for at least an initial pass.
A benefit of this (beyond pure readability) should be that we improve the
clang-tidycoverage for other checks - such asbugprone-unchecked-optional-access- to silently fail, missing clear errors. By simplifying functions, reducing the scope that these checks need to analyse, we should get better coverage. One frustrating niggle is that it's not clear to me what a "safe" threshold for this is - the "cognitive complexity" does not directly correspond with the flow-analysis complexity that causes these checks to fail, and we don't know what threshold the checks fail at. But these measures are likely correlated, and we can do some work to validate where certain checks are and are not being run.Remaining work after #8504
The check is already enabled by #7995 with
Threshold=50andIgnoreMacros=true. The remaining work is to remove the baseline suppressions through behaviour-preserving refactors.Inventory checked against
mainat a6fe2f2 on 2026-10-05, excludingNodeEndpoints::init_handlers, which the now-merged #8504 addresses. The post-#8504 backlog contains six top-level functions plus two nested callbacks, covered by seven suppression sites across five files. The two mechanical follow-up PRs below are open, not yet merged.Scores below are the historical measurements at bc7f0cf from the earlier comment, not newly measured scores on the current revision. Each refactor should verify that the registration function and all extracted callbacks meet the threshold, rather than merely moving the suppression.
init_handlerssetup_basic_hooksinitiate_join_unsafeand its response/task callbacksfill_range_response_from_fileprocess_command_innerdo_execute_requestAlready addressed since the earlier inventory: the two unit-test suppressions (#8482), governance ack handlers (#8484), service-state handlers (#8486), proposal handlers (#8487), file-serving handler registration (#8488), and the programmability constructor (#8489). Node endpoint registration is addressed by #8504.
Actual clang-tidy 18.1.8 measurements with
IgnoreMacros=truein the open follow-ups: #8508 reduces logginginit_handlersfrom 277 to 0, with all 41 extracted handlers scoring at most 31; #8509 reducessetup_basic_hooksfrom 66 to 0, with all six extracted callbacks scoring at most 12. All retained registration adapters in both PRs score 0. Neither PR changes thresholds or adds complexity suppressions.After these two PRs land, the three control-flow refactors and the partially mechanical join-response item remain for assessment.