COMP: Declare the exported functions that no header declares - #37
hjmjohnson merged 4 commits into
Conversation
82ced2a to
1fcc7b4
Compare
|
Rebased onto current
The symbol baseline landed in #77, so this PR's central claim — that declaring rather than Comments and commit message trimmed to the prose budgetThe four six-line comment blocks are now one line each: /* Exported but unpublished; static would drop symbols downstream links against. */The ABI constraint is the load-bearing part and is kept; the rest restated the diff. The The commit body was cut from 18 lines to 6, dropping the table of function names that the diff already shows, and the Two symbols still worth a decision
Separately, |
1fcc7b4 to
373e2cb
Compare
|
Restructured into three commits, because the fourteen functions are not one situation.
Verified on Ubuntu 24.04 with GCC 13.3: warnings 15 → 1, 345/345 tests, symbol baseline updated and matching. The one remaining warning is the Why these ten belong in the headers
A public getter with a private setter is the clearest case:
These declarations are additive. No symbol changes, and no caller has to be edited. Why these two are static
This removes two symbols from The route back out is written at the definitions: /* static: if a downstream project needs these, declare them in fslio.h with FSL_API. */Why nifti_fileexists is left aloneIt is defined with external linkage in both It is also not an isolated case. The symbol baseline shows 100 names exported by both libraries: awk '{print $2}' cmake/exported_symbols_linux.txt | sort | uniq -d
Fourth commit
|
FslGetHdrImgNames and FslSetIntensityScaling are defined here but declared nowhere, while FslGetIntensityScaling and FslInit are already published. axml_recur_find_xml sits beside axml_recur in afni_xml.h the same way. The upstream fslio was deleted in 2015 in favour of a C++ replacement, so this copy is the surviving one and its header is ours to correct. Additive; no symbol changes.
Nine functions in fslio.c have external linkage and no declaration in any header. Nothing in the tree calls them across a translation unit, AFNI's vendored copy never calls them, and no public source outside a vendored copy of this file references them, so a caller would have had to declare them itself. Collect their prototypes in one block so the boundary between internal and published is visible in one place. FslSetVoxUnits and FslGetVoxUnits have no caller at all and are left under #if 0 rather than deleted; FslSetTimeUnits and FslGetTimeUnits are published, so the asymmetry is worth keeping visible. Removes nine symbols from libfslio, which the baseline records.
nifti1_io.c and nifti2_io.c each define nifti_fileexists with external linkage, so libniftiio and libnifti2 export the same name and ELF link order decides which one a caller linking both resolves to. Declaring it locally silences the warning without choosing between publishing it and making it static; that choice needs the duplication settled first, and it covers a hundred names, not this one.
Keeps FslGetAuxFile and FslSetAuxFile deriving the same length so the pair cannot drift if the field width changes. sizeof is 24, so the copy still writes at most 23 bytes.
373e2cb to
15db8e0
Compare
|
Restructured again, on evidence rather than on the internal-consistency argument I used before. Three functions are declared; nine are given internal linkage; Verified on Ubuntu 24.04 with GCC 13.3: warnings 15 → 1, 345/345 tests, exported symbols 448 → 439 with the baseline updated in the same commit. The remaining warning is the
Why only three are declaredThe previous revision declared ten, reasoning that each had a published counterpart. That argument does not survive checking who actually calls them.
The upstream this was copied from no longer exists
The last change to So there is no newer version to track and no upstream to send this to. This copy is the surviving one, which is what makes correcting its header a maintenance decision rather than a divergence. Two functions left under #if 0
They are wrapped in What nifti_fileexists is waiting onIt is defined with external linkage in both That choice should not be made one symbol at a time. The baseline shows 100 names exported by both libraries, and 60 of the 100 have different implementations, because Still warns under FSLSTYLE=ON, and not addressed here
Neither is in scope here. Both became visible only once #75 made CI compile those paths. |
064dc2d
into
InsightSoftwareConsortium:master
|
The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: This PR's commits on the rewritten
The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves. |
2 participants
Fourteen functions are compiled into libniftiio, libnifti2, libfslio and
libcifti with external linkage, and not one is declared in any header --
so they are exported, linkable, and invisible.
nifti_fileexists nifti1_io.c and nifti2_io.c
axml_recur_find_xml afni_xml.c
FslIsValidFileType, FslGetFileType2, fslio.c
FslFileType, FslGetReadFileType,
FslGetHdrImgNames, FslInit4Write,
fsl_fileexists, check_for_multiple_filenames,
FslSetVoxUnits, FslGetVoxUnits,
FslSetIntensityScaling
Each gets a prototype in its own translation unit, which is the only fix
that leaves the interface alone.
staticis what clang-tidy'smisc-use-internal-linkage wants, and it would delete all fourteen symbols
from the shared libraries; moving them to a public header would enlarge
the API with functions nobody has reviewed as public.
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