Repository navigation
Enable clang-tidy's readability-function-cognitive-complexity check #7358
Description
Diff to enable this looks something like this:
diff --git a/.clang-tidy b/.clang-tidy
index c0a984def..941f67e03 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -39,11 +39,16 @@ Checks: >
-performance-no-int-to-ptr,
portability-*,
readability-*,
- -readability-function-cognitive-complexity,
-readability-identifier-length,
-readability-avoid-nested-conditional-operator,
-readability-convert-member-functions-to-static,
+CheckOptions:
+ - key: readability-function-cognitive-complexity.IgnoreMacros
+ value: 'true'
+ - key: readability-function-cognitive-complexity.Threshold
+ value: '50'
+
WarningsAsErrors: '*'
HeaderFilterRegex: '?!(3rdparty)'
FormatStyle: 'file'
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-tidy coverage for other checks - such as bugprone-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=50 and IgnoreMacros=true. The remaining work is to remove the baseline suppressions through behaviour-preserving refactors.
Inventory checked against main at a6fe2f2 on 2026-10-05, excluding NodeEndpoints::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.
| Baseline score | Function | Location | Remaining approach | Follow-up |
|---|---|---|---|---|
| 277 | Logging sample init_handlers |
samples/apps/logging/logging.cpp:593 | Mechanical: extract inline endpoint handlers, preserving registration, authentication, forwarding, and handler behaviour. | #8508 (open) |
| 66 | setup_basic_hooks |
src/node/node_state.h:3277 | Mechanical: extract hook callbacks, preserving hook order, capture lifetimes, and commit/rollback behaviour. | #8509 (open) |
| 125; nested callbacks 89 and 61 | initiate_join_unsafe and its response/task callbacks |
src/node/node_state.h:1382 | Partly mechanical: callback extraction reduces the outer score, but the callbacks themselves still need decomposition of join-response handling. | Not started |
| 69 | fill_range_response_from_file |
src/node/rpc/file_serving_handlers.h:173 | Control-flow decomposition: isolate range parsing/response construction while preserving validation and HTTP semantics. | Not started |
| 90 | process_command_inner |
src/node/rpc/frontend.h:707 | Careful request-path decomposition: preserve authentication, forwarding, transaction/retry behaviour, and error paths. | Not started |
| 102 | do_execute_request |
src/js/registry.cpp:58 | Careful JS execution-path decomposition: preserve exception/timeout handling, ownership, and response semantics. | Not started |
Already 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=true in the open follow-ups: #8508 reduces logging init_handlers from 277 to 0, with all 41 extracted handlers scoring at most 31; #8509 reduces setup_basic_hooks from 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.
Activity
achamayou commentedon Oct 1, 2026
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:init_handlersinit_handlersinit_proposals_handlersinitiate_join_unsafe(+ nested lambdas, 89 and 61)init_service_state_handlersdo_execute_requestprocess_command_innerinit_ack_handlersfill_range_response_from_file/init_file_serving_handlerssetup_basic_hooksProgrammabilityHandlersconstructorA 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.achamayou commentedon Oct 6, 2026
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.NodeState::initiate_join_unsafe, src/node/node_state.h:1382StartupSeqnoIsOld, 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.BaseDynamicJSEndpointRegistry::do_execute_request, src/js/registry.cpp:58RpcFrontend::process_command_inner, src/node/rpc/frontend.h:707Txownership hand-off, acquire ordering of consensus/history loads, and the abort on serialisation failure.can_replicate()calls.fill_range_response_from_file, src/node/rpc/file_serving_handlers.h:173Rangeheader parsing (44).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.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.
achamayou commentedon Oct 7, 2026
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.NodeState::initiate_join_unsafe, src/node/node_state.h:1388RpcFrontend::process_command_inner, src/node/rpc/frontend.h:710can_replicate()calls.fill_range_response_from_file, src/node/rpc/file_serving_handlers.h:173Rangeheader parsing (44).main) renames this function to a templatedfill_range_responseand rewrites its read path.Lessons from #8511 for the remaining PRs:
mainapart from the helper calls and return statements.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.
achamayou commentedon Oct 12, 2026
Remaining work and plan (as of 2026-10-12)
Three functions are still flagged on
mainat 73d31b4. They have four suppression sites across three files: oneNOLINTNEXTLINEper function, plus aNOLINTBEGIN/ENDaround the join response callback. All three bodies are unchanged since the 2026-10-07 update, so the scores still hold (clang-tidy 18.1.8, repository.clang-tidy, suppressions disabled).NodeState::initiate_join_unsafe, node_state.h:1395RpcFrontend::process_command_inner, frontend.h:710fill_range_response_from_file, file_serving_handlers.h:173Rangeheader parsing (44). The other 25 is spread over the digest,If-None-Matchand response-building code.Two corrections to the previous breakdowns: there are seven catch clauses in
process_command_inner, not nine, and the join task's transport errors cost 8, not 7. The totals are unchanged.Plan
1.
initiate_join_unsafe(125 -> 35, callback 18, task 9)NodeStatemember functions and leave the lambdas in place: transport errors (4), 4xx handling (8), non-200/redirect (4) and trusted-join completion (17). Call sites becomeif (cond) { helper(...); return; }.NodeState::lock(use the_unsafesuffix); the trusted-join call stays inside the existingtry(it throwsstd::logic_error); the fatal snapshot-installreturnmust still skip the rest of the completion.2.
process_command_inner(90 -> 44)FAIL_CONFLICTis the retry case), the redirect/forwarding block (16) and the backpressure check (4). The first two alone give 48, which is too thin; all three give 44.attemptsand every catch clause in the function (CompactedVersionConflictstayscontinue,KvSerialiserExceptionstays an abort). PreserveTxownership (moved intoEndpointContextImpl, plus theowned_tx == nullptrcheck) and the order: execute, session consistency,should_apply_writes, commit, local-commit handler,try_emit_signature.can_replicate()calls, which is a trivial rebase.3.
fill_range_response_from_file(69 -> 28)Rangeparsing into a pure parser with noctx(30), returning the half-open range or an error message. The caller keepsset_error(400, InvalidHeaderValue, ...), thereturnandhas_range_header.SIZE_MAXoverflow) and therange_start == range_endrejection. Add unit tests for the parser, which becomes directly testable.fill_range_response(75, or 34 with the same extraction), keeps the suppression and leaves theRangeblock untouched. Either order works; apply the change to whichever name exists.Sequencing and acceptance
Closes #7358.The projected scores come from a re-implementation of the clang-tidy algorithm that reproduces the measured 125, 90 and 69 (and 89 and 61 for the join lambdas) exactly, applied to the refactored code including call sites. They are estimates, so confirm them with clang-tidy in each PR.