Conversation
5c593fc to
627fc15
Compare
0b782cc to
b75b1e1
Compare
b75b1e1 to
6ff9ffa
Compare
6ff9ffa to
ef0af48
Compare
|
Rebased onto current The message said the job "cannot pass today" and listed 17 remaining warning sites blocked on three other PRs. Those have since landed, so I measured rather than repeating the claim:
What changed in the commit message, and why
Author, committer, and dates are unchanged. Two things worth a maintainer decisionThe The |
7d841c7 to
bb8e9af
Compare
bb8e9af to
5fdc07c
Compare
5fdc07c to
498fd73
Compare
A new workflow with two jobs, so that the clang-tidy configuration and
the shared warning set are exercised rather than merely present.
clang-tidy configures with CMAKE_EXPORT_COMPILE_COMMANDS
and runs run-clang-tidy. It reports findings
without failing; removing the '|| true' is the
last step of that work, once the remaining
checks are clean.
warnings-as-errors a gcc and a clang job, each configuring with
NIFTI_WARNINGS_AS_ERRORS=ON, building and
testing.
The warning sites that once blocked the second job have since been
fixed: the sign conversions in the tools, the transposed calloc
arguments, and the ambiguous-filename rewrite all landed separately.
(cherry picked from commit bb8e9af)
(cherry picked from commit 5fdc07c)
A new workflow with two jobs, so that the configuration added in #32 and
#33 is exercised rather than merely present.
clang-tidy configures with CMAKE_EXPORT_COMPILE_COMMANDS and
runs run-clang-tidy. It reports findings without
failing: 331 on master with #32's configuration,
falling to 198 once the rest of this set of PRs is
applied, across 16 checks. They are all real
defect classes rather than style noise, and want
fixing one class at a time. Removing the
'|| true' is the last step of that work.
warnings-as-errors a gcc and a clang job, each configuring with
NIFTI_WARNINGS_AS_ERRORS=ON, building and testing.
Stated plainly, because it decides when this can be merged: the
warnings-as-errors job cannot pass today. With every PR in this set
applied, 17 warning sites remain, and every one of them is on a line an
open PR already rewrites, so they were deliberately left alone:
clang sees 9 of the 17; it has no -Wcalloc-transposed-args.
This PR is therefore blocked on #11, #22 and #24. Once those merge the
count is zero and the job goes green; if they are not going to merge, say
so and I will take those lines in a follow-up instead.
Depends on #32 for .clang-tidy and #33 for NIFTI_WARNINGS_AS_ERRORS.
Neither is included here, because neither shares a file with this one.
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