BUG: Give the statement macros a do/while(0) body and drop the stray semicolons - #49
Conversation
11aab19 to
0dcf89b
Compare
gdevenyi
changed the title
8a61032 to
686dd8c
Compare
686dd8c to
c6e71fb
Compare
c6e71fb to
d220497
Compare
|
Rebased onto I checked every call site for an observable behavior change. There is none — this is prophylactic. Evidence below. The call-site check (the interesting part)A
So there is no red to prove. If one of those greps had hit, that would have been a real bug and a real test; it did not. What is measurable is the warning count, with clang
Class is BUILD: 171 warnings to 0, no runtime seam. Diff reworked to cut churnThe reviewed version inserted a separate Each macro now changes exactly two lines — Message cleanupRemoved the Build and test
|
d220497 to
5982fc1
Compare
|
Rebased onto current
What the first commit changesThree kinds of edit, all behavior-neutral:
This is prophylactic, not a repair. I checked before claiming it: no
What the second commit changes, and why it is separateWithin a single macro the backslashes sat at four different columns — 62, 64, 73 and 86 — with one past 80. Each macro is now aligned to its own width, one space past its longest line. Aligning per block rather than to a global column matters: the 86-column Only the seven macros the first commit already rewrites are touched. It is a separate commit because it is 66 lines of pure whitespace. Keeping it apart leaves the code change readable at 18 lines with zero style noise, and lets either commit be taken without the other. Verification
No test is added and none applies. The change is behavior-neutral by construction: |
A macro whose body is a bare { ... } block cannot be used as a
statement:
if( cond )
FSLIOERR(...); /* the ; ends the if, the block runs */
else /* ...and this is a syntax error */
No current call site is written that way -- FSLIOERR has no `else`
after it anywhere, and none of the nifti_*_test macros appears in an
if or else arm -- so the change is prophylactic and nothing observable
moves. The next person to write one, though, gets a syntax error a
long way from the cause. clang reports the call sites as
-Wextra-semi-stmt, because the trailing semicolon at each is an empty
statement.
Wrapped: FSLIOERR in fslio.c and the seven nifti_*_test macros in
nifti_tester001.c. The NT_FILL and NT_MAT* families in the tool
headers already have do/while bodies.
Two genuine empty statements go too: a doubled semicolon inside
unescape_string() in both io files, and a stray semicolon after the
closing brace of an if block in FslClose().
FSLIOERR is worth calling out: it expands to fprintf plus
exit(EXIT_FAILURE), and of its roughly fifty call sites several are
the whole body of a braceless if. Those are correct today only
because the macro always exits.
The backslashes sat at four different columns within a single macro, one of them past 80. Each macro is now aligned to its own width, one space past its longest line, so a block stays as narrow as its content allows rather than being widened by one long line. Only the seven macros the preceding commit gives a do/while(0) body are touched.
6f451c0 to
a91aa16
Compare
3f14c7e
into
InsightSoftwareConsortium:master
3 participants
A macro whose body is a bare { ... } block cannot be used as a statement:
None of the current call sites is written that way, so nothing is broken
today; the next person to write one gets a silent behaviour change or a
compile error a long way from the cause. clang reports these as
-Wextra-semi-stmt, because the trailing semicolon at each call site is an
empty statement.
Wrapped: the FSLIOERR macro in fslio.c and the seven nifti__test
macros in nifti_tester001.c. The NT_FILL and NT_MAT families in the
tool headers already have do/while bodies and need nothing.
Two genuine empty statements are removed as well: a doubled semicolon
inside unescape_string() in both io files, and a stray semicolon after
the closing brace of an if block in FslClose().
FSLIOERR is worth calling out. It expands to
and there are roughly fifty call sites, several of them the whole body of
an if with no braces. Those are correct today only because the macro
always exits.
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