COMP: Add a shared compiler warning set - #33
Conversation
|
Taking this over to split it, with thanks — the flag selection here is good, and the "deliberately NOT enabled" reasoning for The problem is not which flags; it is that enabling 17 at once forces a choice between a wall of warnings nobody acts on, or Policy for this repo going forward: a warning flag is added only when the tree is already at zero under it, and CI is green at every PR in the sequence. So this PR becomes the clean set — the flags measured at zero, which can land now — and a companion draft PR carries the future set with the work each one needs. Both are rebased onto current Census — measured, not estimatedAppleClang 21, Release,
Worth flagging: the Clang-only block in this PR is not clean — six of its seven flags fire; only What this PR becomesFourteen flags, verified by a local build with The file keeps your options, your documentation, and your exclusion rationale, and gains a Two branches held back for lack of CI, not for lack of cleanlinessThe GCC-only block is unmeasured. GNU 13.3.0 does run in CI ( MSVC Incidentally, the matrix has a redundancy worth reclaiming: the two The promotion sequence, once this landsOne flag (or small group) per PR, each preceded by the PR that fixes its warnings, CI green after each:
Census caveats
|
ebd619b to
040d983
Compare
Add cmake/nifti_warnings.cmake with the warning flags the tree is already at zero under, so that a warning means a new defect rather than more noise. Fourteen flags are enabled: thirteen common to GCC and Clang, plus -Wcomma on Clang. Verified with NIFTI_WARNINGS_AS_ERRORS=ON in both the default and the USE_CIFTI_CODE/USE_FSL_CODE/FSLSTYLE configurations: zero compiler warnings. The flags that are wanted but not yet earned are recorded in a FUTURE SET comment block with their measured hit counts and the census command that produces them. They are promoted one at a time, each only after the change that fixes its warnings has landed, so CI stays green at every step. The GCC-only and MSVC branches are left empty: the GCC block has never been enabled anywhere so its counts are unknown, and no workflow builds on Windows at all. Co-Authored-By: Gabriel A. Devenyi <3001850+gdevenyi@users.noreply.github.com>
040d983 to
e506a75
Compare
|
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. |
2 participants
The CMake build set no warning flags at all. The only warning flags in
the project lived in the CI workflow's environment, which meant they
applied to nobody's local build and, since that workflow had never run,
to nothing at all.
cmake/nifti_warnings.cmake enables a set the project can realistically
hold at zero, so a future warning means a new defect rather than more
noise. Warnings are not errors yet; the categories below are fixed one
per commit in the branches that follow, and -Werror is turned on at the
end via NIFTI_WARNINGS_AS_ERRORS.
Baseline as of this commit, whole tree, cifti and fsl enabled:
gcc 16.1.1 clang 22.1.8
223 -Wsign-conversion 272 -Wextra-semi-stmt
19 -Wcast-align 222 -Wsign-conversion
14 -Wmissing-prototypes 31 -Wshorten-64-to-32
12 -Wcalloc-transposed-args 19 -Wcast-align
9 -Wcast-qual 14 -Wmissing-prototypes
3 -Wsign-compare 11 -Wconditional-uninitialized
1 -Wstringop-truncation 9 -Wcast-qual
1 -Wlogical-op 3 -Wsign-compare
2 -Wmissing-variable-declarations
1 -Wnewline-eof
Three tempting flags are deliberately left out, documented in the module:
-Wdouble-promotion (~240 hits, silencing them means switching to sqrtf()
and changing the numerical results of the quaternion code),
-Wfloat-equal (~150, mostly deliberate tests against exact 0.0), and
-Wconversion (subsumed by the two above plus -Wsign-conversion).
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