Skip to content

COMP: Build as ISO C11 with no compiler extensions - #38

Open
gdevenyi wants to merge 1 commit into
InsightSoftwareConsortium:ci/guard-cmp0169from
gdevenyi:pr/c11-standard
Open

gdevenyi wants to merge 1 commit into
InsightSoftwareConsortium:ci/guard-cmp0169from
gdevenyi:pr/c11-standard

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

The build never set a C standard, so it used the compiler default. Set
CMAKE_C_STANDARD 11 with C_EXTENSIONS OFF.

Two POSIX dependencies were being satisfied silently by the gnu default
and now have to be requested:

cifti/afni_xml.c strdup(), 3 uses. Without a declaration this was
an implicit function declaration returning int
assigned to char * -- an error in C99 onward.
nifti1_io.c, popen()/pclose() in the optional PIGZ write path,
nifti2_io.c only compiled with -DFSLSTYLE_PIGZ_SUPPORT=ON.

Each file defines _POSIX_C_SOURCE 200809L ahead of its includes, so the
dependency is visible where it is taken.


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 pushed as 82379c9. Applied cleanly, no conflicts. No test added: setting CMAKE_C_STANDARD is a compiler-invocation change, and the two _POSIX_C_SOURCE defines exist to keep declarations visible — if either were wrong the build would fail, which is the check.

Built and tested in two configurations, because the popen()/pclose() half of this change only compiles under FSLSTYLE_PIGZ_SUPPORT:

  • Release + applications + nifti2 + cifti + fsl: 100% tests passed, 0 tests failed out of 362
  • the same plus -DFSLSTYLE=ON -DFSLSTYLE_PIGZ_SUPPORT=ON: 100% tests passed, 0 tests failed out of 364 — this is the configuration that actually compiles the PIGZ write path in nifti1_io.c and nifti2_io.c, so the _POSIX_C_SOURCE define there is exercised rather than assumed.

Both on macOS/AppleClang, where _POSIX_C_SOURCE is the riskiest (it can hide BSD extensions as well as reveal POSIX ones). Clean.

What changed in this update
  • Rebased onto master; no conflicts.
  • Commit message: dropped the Co-Authored-By: naming an AI tool and the Claude-Session: URL. Nothing else in the message needed changing.
  • Trimmed the three new source comments. Each was a three-line block; the versions that narrated what the code used to rely on ("instead of relying on a global -std=gnu11") are gone, leaving one line each stating why the define is there. Same for the CMakeLists.txt block. No functional change — 11 added lines before and after, and identical ignoring whitespace.

Comment thread cifti/afni_xml.c Outdated
Comment thread CMakeLists.txt
@hjmjohnson
hjmjohnson force-pushed the pr/c11-standard branch 2 times, most recently from c8ae7ee to 49b48b4 Compare September 23, 2026 11:20
@hjmjohnson
hjmjohnson marked this pull request as draft September 23, 2026 12:06
The build never set a C standard, so it used the compiler default.  Set
CMAKE_C_STANDARD 11 with C_EXTENSIONS OFF.

Two POSIX dependencies were being satisfied silently by the gnu default
and now have to be requested:

  cifti/afni_xml.c     strdup(), 3 uses.  Without a declaration this was
                       an implicit function declaration returning int
                       assigned to char * -- an error in C99 onward.
  nifti1_io.c,         popen()/pclose() in the optional PIGZ write path,
  nifti2_io.c          only compiled with -DFSLSTYLE_PIGZ_SUPPORT=ON.

Each file defines _POSIX_C_SOURCE 200809L ahead of its includes, so the
dependency is visible where it is taken.

(cherry picked from commit 49b48b4)
@hjmjohnson
hjmjohnson changed the base branch from master to ci/guard-cmp0169 September 24, 2026 11:59
@hjmjohnson
hjmjohnson marked this pull request as ready for review September 24, 2026 12:00
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.

3 participants