Skip to content

BUG: Stop casting away const in fslio and cifti - #45

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-cast-qual
Sep 22, 2026
Merged

hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/fix-cast-qual

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

Nine -Wcast-qual warnings. Casting away const is how a read-only
contract turns into a write to memory the caller thought was safe, so
each was looked at rather than silenced.

fslio.c, FslGetFileType2()
mutablefslio = (FSLIO )fslio; / dodgy and will generate warnings */
mutablefslio->niftiptr->nifti_type = ...;

The comment is right that it looks dodgy, but the cast was never
needed.  `const FSLIO * fslio` makes the *member* niftiptr const --
its type is `nifti_image * const` -- while what it points at stays
fully mutable.  The assignment is legal as written, so the cast and
the local both go.

fslio.c FslWriteVolumes(), cifti axio_num_tokens(), text_to_i64(),
text_to_f64()
Pointers cast to char * and then only read: walked, indexed, or
handed to strtoll()/strtod(), which take const char *. Declared
const, casts removed.

cifti strip_whitespace()
Returned (char *)str on its early-exit paths, handing the caller a
writable pointer to the const string it passed in. The function is
static, so its return type is nobody else's business; it now returns
const char *.


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 master (b4876bf), cleaned the commit message, and tidied three small things in the diff itself.

No regression test applies: every hunk is type-qualifier-only and generates identical code. Details below.

Message cleanup

Removed the Co-Authored-By: trailer naming an AI tool and the Claude-Session: URL. Body otherwise unchanged. Author and author date preserved (Gabriel A. Devenyi, 2026-08-14).

Three fixes to the diff
  • fslio.c: const char *inbuf; was indented 2 columns instead of 6, next to the char *tmpbuf; it was split from.
  • fslio.c: inbuf = (const char *) buffer; — buffer is already const void *, which converts implicitly. Dropped the redundant cast, consistent with the rest of the change.
  • fslio.c: shortened the explanatory comment to one line.
  • afni_xml.c: the forward declaration kept the old column alignment after char became const char, leaving a run of stray spaces. Collapsed to a single space.
Why there is no red-green test

Every hunk changes only type qualifiers on locals, a static function's return type, and the removal of casts. No expression's value, no control flow, and no emitted instruction changes:

  • FslGetFileType2() performed mutablefslio->niftiptr->nifti_type = ...; it now performs fslio->niftiptr->nifti_type = .... Same store, same address — const FSLIO * only makes the niftiptr member itself const.
  • axio_num_tokens(), text_to_i64(), text_to_f64() only ever read through the pointers.
  • strip_whitespace() is static; its three early-exit paths return the same address as before, just typed const char *.

A ctest cannot distinguish the two trees. The class is BUILD/COSMETIC: nine -Wcast-qual warnings removed, no runtime seam.

Build and test
  • Release, NIFTI_BUILD_APPLICATIONS=ON USE_NIFTI2_CODE=ON USE_CIFTI_CODE=ON USE_FSL_CODE=ON: 100% tests passed, 0 tests failed out of 362.
  • Same plus -DFSLSTYLE=ON (this PR touches fslio.c): 100% tests passed, 0 tests failed out of 364.

Whitespace churn check: git diff master HEAD and git diff -w master HEAD both report 18 added lines.

Comment thread fsliolib/fslio.c Outdated
Nine -Wcast-qual warnings.  Casting away const is how a read-only
contract turns into a write to memory the caller thought was safe, so
each was looked at rather than silenced.

fslio.c, FslGetFileType2()
    mutablefslio = (FSLIO *)fslio; /* dodgy and will generate warnings */
    mutablefslio->niftiptr->nifti_type = ...;

    The comment is right that it looks dodgy, but the cast was never
    needed.  `const FSLIO * fslio` makes the *member* niftiptr const --
    its type is `nifti_image * const` -- while what it points at stays
    fully mutable.  The assignment is legal as written, so the cast and
    the local both go.

fslio.c FslWriteVolumes(), cifti axio_num_tokens(), text_to_i64(),
text_to_f64()
    Pointers cast to char * and then only read: walked, indexed, or
    handed to strtoll()/strtod(), which take const char *.  Declared
    const, casts removed.

cifti strip_whitespace()
    Returned `(char *)str` on its early-exit paths, handing the caller a
    writable pointer to the const string it passed in.  The function is
    static, so its return type is nobody else's business; it now returns
    const char *.
@hjmjohnson
hjmjohnson requested a review from seanm September 22, 2026 20:36
@hjmjohnson
hjmjohnson merged commit 0a9aa9c into InsightSoftwareConsortium:master Sep 22, 2026
26 checks passed
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