Skip to content

COMP: Add CI jobs for clang-tidy and for building with -Werror - #59

Open
gdevenyi wants to merge 2 commits into
InsightSoftwareConsortium:ci/install-linking-srcdirfrom
gdevenyi:pr/ci-static-checks
Open

gdevenyi wants to merge 2 commits into
InsightSoftwareConsortium:ci/install-linking-srcdirfrom
gdevenyi:pr/ci-static-checks

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

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:

 9  -Wsign-conversion         5 in fsliolib/fslio.c (#22),
                              2 in nifti2/nifti2_io.c (#24),
                              2 in nifti_tester001.c (#24)
 8  -Wcalloc-transposed-args  nifti_findhdrname() and
                              nifti_findimgname() in both libraries,
                              which #11 rewrites

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=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself 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, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 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 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_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 master and 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

This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Rebased onto current master (b4876bf) and the commit message rewritten. The blocker described in the original message is gone: the warnings-as-errors job passes today.

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:

Compiler NIFTI_WARNINGS_AS_ERRORS=ON on b4876bf
GCC 13.3 (Ubuntu 24.04, same major as ubuntu-latest) build rc=0
AppleClang build rc=0, 363/363 tests
What changed in the commit message, and why

master's last 44 commits were rewritten on 2026-09-22 to drop trailers that do not belong in permanent history, and this PR is held to the same standard:

  • removed Co-Authored-By: naming an AI tool, and the Claude-Session: URL
  • removed eight bare #NNN cross-references. This is a fork, so a number means one PR here and a different one upstream; when a commit travels to NIFTI-Imaging/nifti_clib or into a vendored copy in ITK, AFNI, FSL, dcm2niix or Slicer, the number points at the wrong thing.
  • rewrote the blocked-on paragraph, which was no longer true

Author, committer, and dates are unchanged.

Two things worth a maintainer decision

The clang-tidy job cannot fail. It ends in || true, so it reports findings and always exits 0. That is a deliberate choice stated in the message, and reasonable while the tree is not clean — but it means the job guards nothing today. A separate, cheap guard that can fail is clang-tidy --verify-config, which catches exactly the configuration rot that motivated reworking .clang-tidy. Note it exits 0 whether or not the config is valid, so the step has to assert on its output text rather than on $?.

The warnings-as-errors job is green by virtue of the runner's compiler. It passes on GCC 13.x and AppleClang. Under GCC 16 there is still one site, fsliolib/fslio.c:936: 'nx' may be used uninitialized. So the job will go red the day ubuntu-latest moves to a newer GCC. Either fix that site first, or pin the compiler, or accept that the job tracks the runner.

hjmjohnson and others added 2 commits September 24, 2026 10:09
%p takes a pointer to void. fname was passed as const char *, while its
two siblings on the same line were already cast, so the mismatch was a
single argument. Clang reports it under -Wformat-pedantic.

(cherry picked from commit 7fa06d7)
(cherry picked from commit f7381a3)
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)
@hjmjohnson
hjmjohnson changed the base branch from master to ci/install-linking-srcdir September 24, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants