ENH: Rework the clang-tidy configuration for C11 - #32
Merged
hjmjohnson merged 1 commit intoSep 21, 2026
Merged
hjmjohnson merged 1 commit into
hjmjohnson merged 1 commit into
Conversation
This was referenced Aug 15, 2026
This was referenced Aug 15, 2026
The .clang-tidy file has been in the tree since 2025 but nothing has ever
run it, and it shows: two of its entries name clang-analyzer checks that
upstream renamed some releases ago, so --verify-config warned about them.
Run over the whole tree it produced 1697 findings, of which the great
majority came from three style checks. At that ratio nobody reads the
output.
Retuned for what this project actually is: C11, with a shipped ABI.
Measured with clang-tidy 22.1.8, USE_FSL_CODE=ON and USE_CIFTI_CODE=ON,
it now gives 331 findings across 15 checks, every one a defect class:
170 bugprone-macro-parentheses
44 bugprone-unchecked-string-to-number-conversion
25 bugprone-multi-level-implicit-pointer-conversion
19 readability-use-concise-preprocessor-directives
16 bugprone-switch-missing-default-case
13 readability-redundant-casting
5 bugprone-suspicious-string-compare
4 bugprone-suspicious-realloc-usage
3 readability-redundant-control-flow
2 readability-suspicious-call-argument
2 bugprone-misplaced-widening-cast
1 each: readability-implicit-bool-conversion,
readability-function-size, misc-redundant-expression,
bugprone-float-loop-counter
bugprone-macro-parentheses is re-enabled. It had been switched off, but
it is the check that catches the real precedence bugs in the NT_DT_*,
QSTR and NT_FILL macro families.
misc-use-internal-linkage is switched off permanently, with the reason
recorded in the file. It flags 29 functions, 13 of which are symbols
currently exported by libniftiio, libfslio and libcifti. Applying its
fix-it would delete them from the shared libraries. That it looks like a
tidy-up is exactly what makes it dangerous, so the file says so next to
the check rather than leaving the next person to rediscover it.
Every disabled check carries its finding count and the reason, so the
list can be re-argued from evidence instead of taste.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
gdevenyi
force-pushed
the
pr/clang-tidy-c11-config
branch
from
August 15, 2026 04:52
689ca9e to
d9a3516
Compare
gdevenyi
marked this pull request as ready for review
August 15, 2026 05:27
hjmjohnson
approved these changes
Sep 21, 2026
Member
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commit on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
This was referenced Sep 24, 2026
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.
The .clang-tidy file has been in the tree since 2025 but nothing has ever
run it, and it shows: two of its entries name clang-analyzer checks that
upstream renamed some releases ago, so --verify-config warned about them.
Run over the whole tree it produced 1697 findings, of which the great
majority came from three style checks. At that ratio nobody reads the
output.
Retuned for what this project actually is: C11, with a shipped ABI.
Measured with clang-tidy 22.1.8, USE_FSL_CODE=ON and USE_CIFTI_CODE=ON,
it now gives 331 findings across 15 checks, every one a defect class:
170 bugprone-macro-parentheses
44 bugprone-unchecked-string-to-number-conversion
25 bugprone-multi-level-implicit-pointer-conversion
19 readability-use-concise-preprocessor-directives
16 bugprone-switch-missing-default-case
13 readability-redundant-casting
5 bugprone-suspicious-string-compare
4 bugprone-suspicious-realloc-usage
3 readability-redundant-control-flow
2 readability-suspicious-call-argument
2 bugprone-misplaced-widening-cast
1 each: readability-implicit-bool-conversion,
readability-function-size, misc-redundant-expression,
bugprone-float-loop-counter
bugprone-macro-parentheses is re-enabled. It had been switched off, but
it is the check that catches the real precedence bugs in the NT_DT_*,
QSTR and NT_FILL macro families.
misc-use-internal-linkage is switched off permanently, with the reason
recorded in the file. It flags 29 functions, 13 of which are symbols
currently exported by libniftiio, libfslio and libcifti. Applying its
fix-it would delete them from the shared libraries. That it looks like a
tidy-up is exactly what makes it dangerous, so the file says so next to
the check rather than leaving the next person to rediscover it.
Every disabled check carries its finding count and the reason, so the
list can be re-argued from evidence instead of taste.
Interface impact: none. On the union of all these changes, configured with
USE_FSL_CODE=ONandUSE_CIFTI_CODE=ON: all 448 exported symbols acrosslibniftiio,libnifti2,libznz,libfslio,libnifticdfandlibciftiare identical tomasterundernm -D --defined-only, and all ten installed headers are identical undergcc -E -P. Undergcc -dM -Eone macro definition differs, intentionally and only in text: #61 makesFSL_RADIOLOGICALread(-1)so it is safe inside an expression. Its value is still-1, checked by compiling against each installedfslio.hand printing it.Verification. This branch: builds with gcc 16.1.1,
ctestunchanged frommaster(2 of 345 fail onmasteritself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8,ctest345/345 under each, and the whole suite under valgrind memcheck with--trace-children=yesgives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9
-Wsign-conversion(5 infslio.cfor #22, 2 innifti2_io.cand 2 innifti_tester001.cfor #24) and 8-Wcalloc-transposed-argsinnifti_findhdrnameandnifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.One of a set of independent, single-purpose PRs. Each bases on
masterand can be merged on its own, in any order.The full set of PRs (35)
The union of all of them is on the fork as
all-changes, if you want to build and test the lot at once.CI and build
Configuration and documentation
Defects
Warning and check classes